Nonce mechanism lacks uniqueness checks, risking replay attacks and out-of-order execution.

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L137-L168 https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWallet.sol#L145-L152

Vulnerability details

The nonce mechanism in CoinbaseSmartWallet.sol has potential issue related to nonce uniqueness and ordering for non-replayable user operations. The current implementation does not explicitly check the uniqueness of nonces and does not enforce strict ordering, which could lead to replay attacks and out-of-order execution of user operations within the same chain.

Description:

validateUserOp, which is responsible for validating user operations takes the following parameters:

• userOp: The user operation to be validated.
• userOpHash: The hash of the user operation.
• missingAccountFunds: The missing account funds required for the operation.

Loc: validateUserOp

   function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds)
       public
       payable
       virtual
       onlyEntryPoint
       payPrefund(missingAccountFunds)
       returns (uint256 validationData)
   {
       uint256 key = userOp.nonce >> 64;

       // 0xbf6ba1fc = bytes4(keccak256("executeWithoutChainIdValidation(bytes)"))
       if (userOp.callData.length >= 4 && bytes4(userOp.callData[0:4]) == 0xbf6ba1fc) {
           userOpHash = getUserOpHashWithoutChainId(userOp);
           if (key != REPLAYABLE_NONCE_KEY) {
               revert InvalidNonceKey(key);
           }
       } else {
           if (key == REPLAYABLE_NONCE_KEY) {
               revert InvalidNonceKey(key);
           }
       }

   }

The function extracts the key from the upper 192 bits of the userOp.nonce field and uses it to determine whether the user operation is intended for cross-chain replay or not.

a. Nonce Uniqueness:

• The validateUserOp function does not check if the nonce has already been used for non-replayable user operations.

• If the same nonce is used for multiple non-replayable user operations, it could lead to replay attacks within the same chain.

  b. Nonce Ordering:

• The validateUserOp function does not enforce any specific ordering or sequencing of nonces for non-replayable user operations.
• This could potentially allow out-of-order execution of user operations, which may be undesirable in certain scenarios.

Loc: #145-L151

   uint256 key = userOp.nonce >> 64;
   // ...
   if (userOp.callData.length >= 4 && bytes4(userOp.callData[0:4]) == 0xbf6ba1fc) {
       // ...
   } else {
       if (key == REPLAYABLE_NONCE_KEY) {
           revert InvalidNonceKey(key);
       }
   }

These lines extract the key from the userOp.nonce and check if it is a replayable or non-replayable user operation, but they do not perform any checks for nonce uniqueness or ordering.

The issue is the current implementation allows the same nonce to be used for multiple non-replayable user operations, making it vulnerable to replay attacks within the same chain.

• The current implementation does not enforce any specific ordering of nonces for non-replayable user operations, potentially allowing out-of-order execution.

Impact

If the same nonce is used for multiple non-replayable user operations, an attacker can replay a previously executed user operation, potentially leading to unintended or malicious actions.

Recommended Mitigation Steps

a. Nonce Uniqueness:

• Implement a mapping or a data structure to keep track of used nonces for non-replayable user operations.
• Before executing a non-replayable user operation, check if the nonce has already been used and revert if it has.
• Update the mapping or data structure to mark the nonce as used after successfully executing the user operation.

      mapping(uint256 => bool) private usedNonces;

      function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds)
          public
          payable
          virtual
          onlyEntryPoint
          payPrefund(missingAccountFunds)
          returns (uint256 validationData)
      {
          // ...
          if (key != REPLAYABLE_NONCE_KEY) {
              if (usedNonces[userOp.nonce]) {
                  revert NonceAlreadyUsed(userOp.nonce);
              }
              usedNonces[userOp.nonce] = true;
          }
          // ...
      }

b. Nonce Ordering:

• Implement a mechanism to enforce strict ordering of nonces for non-replayable user operations.
• Keep track of the last processed nonce and ensure that the next nonce is strictly greater than the previous one.
• Revert if a user operation with an out-of-order nonce is encountered.

      uint256 private lastNonce;

      function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, uint256 missingAccountFunds)
          public
          payable
          virtual
          onlyEntryPoint
          payPrefund(missingAccountFunds)
          returns (uint256 validationData)
      {
          // ...
          if (key != REPLAYABLE_NONCE_KEY) {
              if (userOp.nonce <= lastNonce) {
                  revert InvalidNonceOrder(userOp.nonce, lastNonce);
              }
              lastNonce = userOp.nonce;
          }
          // ...
      }

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Incremental nonce is not needed here as userOp.nonce is only there to match up with REPLAYABLE_NONCE_KEY after userOpHash is recomputed.

[3docSec]

The logic that the warden expected to find (nonce check + increment) is implemented upstream in the EntryPoint through its parent NonceManager, making the reported attack path not viable by ensuring replay is not possible.

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid