Request SDK - Double Payment Prevention - Implement a Nonce or Idempotency Key at smart contract level

[MantisClone]

Problem

The payment proxy smart contracts will process an accidental "double payment" in which the user submits the same transaction twice.

Proposed Solution

• [ ] Sequential nonce
  • [ ] Smart contract registry of nonces for each payment reference
  • [ ] New payment proxy smart contracts that check the registry before processing payments.
  • [ ] When creating a request, register the payment reference with nonce 0 on the registry on the payment chain.
  • [ ] When paying a request, caller includes the next nonce. Smart contract checks that the nonce is correct.

Details: Brainstorm in the double payment prevention feature

Considerations

• How to handle "In-Memory Requests" (Pay-before-persist)?

  • Proposed: If paymentReference not in registry, skip nonce check. In other words, "In-memory Requests" don't get double payment prevention.

• This solution would not prevent "overpayment" where the user submits a correct nonce, but the amount exceeds the expected amount of the request. This is intentional because overpayment prevention is more complicated to implement. It would require storing the expected amount onchain and crosschain messaging to make it available on the payment chain.

[vrolland]

I see two/three solutions for the proxy with nonce:

1. include the Nonce as a parameter of the payment function of the proxy a. straight forward version b. fallback nonce version
2. Include the Nonce in the hash computing the payment reference

Solutions

1. Clear proxy nonce

a. straight forward version

The nonce is a parameter added to transferFromWithReferenceAndFee() and we check that's hash(_paymentReference, nonce) has not already been used.

    [...]
    mapping(bytes32 => bool) public history;


    function transferFromWithReferenceAndFee(
        address _tokenAddress,
        address _to,
        uint256 _amount,
        bytes calldata _paymentReference,
        uint256 _feeAmount,
        address _feeAddress
        uint16 _nonce
    )  {
        [...]
        // Check if the payment has already been done
        require(history[keccak256(abi.encodePacked(_paymentReference, _nonce))] === false, "duplicated payment");

        // Mark the payment as done
        history[keccak256(abi.encodePacked(_paymentReference, _nonce))] = true;
        [...]
    }

    [...]

pros

• Simple to implement
• Payment research is simple always the same payment reference

cons

• Breaking change in the payment function
• Less privacy
• Gas costly (write in storage)

b. fallback nonce version

Same as before but if the nonce is not provided, we don't check the history.

    [...]
    mapping(bytes32 => bool) public history;


    function transferFromWithReferenceAndFee(
        address _tokenAddress,
        address _to,
        uint256 _amount,
        bytes calldata _paymentReference,
        uint256 _feeAmount,
        address _feeAddress
        uint16 _nonce = 0
    )  {
        [...]
        if(_nonce !== 0) {
            // Check if the payment has already been done
            require(history[keccak256(abi.encodePacked(_paymentReference, _nonce))] === false, "duplicated payment");

            // Mark the payment as done
            history[keccak256(abi.encodePacked(_paymentReference, _nonce))] = true;
        }
        [...]
    }

    [...]

pros

• Simple to implement
• Payment research is simple always the same payment reference
• Gas costly (write in storage) only if nonce is provided
• Breaking change in the payment function only if nonce is used

cons

• Less privacy
• Can add confusion

2. Nonce in payment reference

The nonce is added to the payment reference computation:

last8Bytes(hash(lowercase(requestId + salt + address + nonce)))

    [...]
    mapping(bytes32 => bool) public history;


    function transferFromWithReferenceAndFee(
        address _tokenAddress,
        address _to,
        uint256 _amount,
        bytes calldata _paymentReference,
        uint256 _feeAmount,
        address _feeAddress
    )  {
        [...]
        // Check if the payment has already been done
        require(history[_paymentReference] === false, "duplicated payment");

        // Mark the payment as done
        history[_paymentReference] = true;
        [...]
    }

    [...]

pros

• Simple to implement
• No breaking change for payment function
• more resistant to outrun attack (@david mentionned it)
• more privacy

cons

• Change must be made in the payment reference computation specs
• Payment research more painful (need to compute the payment reference with different nonces)
• Gas costly (write in storage)

My conclusion

I would recommend the 1.a solution. It is the simplest to implement, to use and to track the payments.

We should keep the current version of the proxy for request with a small expected amount and add a new payment network for the high amount requests. This would avoid the ~1$ extra cost for the small expected amount requests.

N.B.: It's just a humble input. 😊

[MantisClone]

Looks good @vrolland. I agree that 1.a solution seems good, with one concern: Should the history mapping be stored in a new registry contract, separate from the payment proxy?

Considerations:

• What if we need to deploy a new version of an existing payment proxy? What if someone makes a partial payment on payment proxy V1 and completes the payment on payment proxy V2 (or vice versa)? A shared paymentReference/nonce registry might be helpful in this case.
• What if a request has meta payment network? In this case, each sub-payment-network has a different paymentReference, so there's no benefit to a shared paymentReference/nonce registry.