Replay attack on UserOperation if the smart account is not deployed before

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol#L64

Vulnerability details

Impact

During handling an UserOperation, if the smart account is not deployed already, it will be created and then the UserOperation will be executed. The vulnerability is that since the smart account is going to be deployed before the execution of the UserOperation, it will bypass the nonce incrementation. This provides an opportunity to apply replay attack to execute the same UserOperation twice.

Replaying a user's transaction can have critical financial impact on the user. For example, transferring fund from the user to the receiver twice instead of once.

Proof of Concept

• Suppose, Alice (a honest user) sends a UserOperation to be executed by the bundler for the first time (I mean the smart account for Alice is not created yet, so the nonce is zero). The gas of this operation will be refunded whether by Alice's smart account or a paymaster (it does not matter for this vulnerability). -The function handleOps will be called by the bundler to validate and execute them. https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L68
• During the account prepayment validation of UserOperation, the Alice's smart account will be deployed if its not created already. https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L295
• Since initCode.length != 0, the following function will deploy the Alice's smart account.

  function _createSenderIfNeeded(uint256 opIndex, UserOpInfo memory opInfo, bytes calldata initCode) internal {
      if (initCode.length != 0) {
          address sender = opInfo.mUserOp.sender;
          if (sender.code.length != 0) revert FailedOp(opIndex, address(0), "AA10 sender already constructed");
          address sender1 = senderCreator.createSender{gas : opInfo.mUserOp.verificationGasLimit}(initCode);
          if (sender1 == address(0)) revert FailedOp(opIndex, address(0), "AA13 initCode failed or OOG");
          if (sender1 != sender) revert FailedOp(opIndex, address(0), "AA14 initCode must return sender");
          if (sender1.code.length == 0) revert FailedOp(opIndex, address(0), "AA15 initCode must create sender");
          address factory = address(bytes20(initCode[0 : 20]));
          emit AccountDeployed(opInfo.userOpHash, sender, factory, opInfo.mUserOp.paymaster);
      }
  }

  https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L261

• After the deployment, the Alice's UserOperation should be validated. https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L319

  function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, address aggregator, uint256 missingAccountFunds)
  external override virtual returns (uint256 deadline) {
      _requireFromEntryPoint();
      deadline = _validateSignature(userOp, userOpHash, aggregator);
      if (userOp.initCode.length == 0) {
          _validateAndUpdateNonce(userOp);
      }
      _payPrefund(missingAccountFunds);
  }

  https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/BaseSmartAccount.sol#L60

• The function above shows that since initCode.length != 0, the validation of nonce will be bypassed, and it will not increment either. So, the nonce will still remain zero.
• Now, the Alice's UserOperation is executed, but the nonce of Alice's smart account is still equal to zero. It means, it is possible to apply replay attack.
• Bob (a malicious user) can sends the same Alice's UserOperation to the bundler's mempool with zero nonce again. Or, he may call the function handleOps himself and gives the same Alice's UserOperation as a parameter to execute Alice's UserOperation again.
• This time, during account prepayment validation, since initCode.length = 0, the _createSenderIfNeeded will not be executed. Instead, during validateUserOp, the function _validateAndUpdateNonce will be called. https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L501
• In this function, it checks that the UserOperation nonce (that is equal to zero, since Bob sends the same Alice's UserOperation again) is equal to the Alice's smart account nonce (that is equal to zero because in the previous UserOperation execution, the nonce was not incremented due to smart account creation bypass). So, it validates this UserOperation again and increments the nonce to 1.

  function _validateAndUpdateNonce(UserOperation calldata userOp) internal override {
      require(nonces[0]++ == userOp.nonce, "account: invalid nonce");
  }

  https://github.com/code-423n4/2023-01-biconomy/blob/5df2e8f8c0fd3393b9ecdad9ef356955f07fbbdd/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L501

Tools Used

Recommended Mitigation Steps

• Whether change the increment order:

  //before
  function _validateAndUpdateNonce(UserOperation calldata userOp) internal override {
      require(nonces[0]++ == userOp.nonce, "account: invalid nonce");
  }
  
  //after
  function _validateAndUpdateNonce(UserOperation calldata userOp) internal override {
      require(++nonces[0] == userOp.nonce, "account: invalid nonce");
  }

• Or, remove the condition initCode.length == 0

  //before
  function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, address aggregator, uint256 missingAccountFunds)
  external override virtual returns (uint256 deadline) {
      _requireFromEntryPoint();
      deadline = _validateSignature(userOp, userOpHash, aggregator);
      if (userOp.initCode.length == 0) {
          _validateAndUpdateNonce(userOp);
      }
      _payPrefund(missingAccountFunds);
  }
  
  //after
  function validateUserOp(UserOperation calldata userOp, bytes32 userOpHash, address aggregator, uint256 missingAccountFunds)
  external override virtual returns (uint256 deadline) {
      _requireFromEntryPoint();
      deadline = _validateSignature(userOp, userOpHash, aggregator);
      _validateAndUpdateNonce(userOp);
      _payPrefund(missingAccountFunds);
  }

[c4-judge]

gzeon-c4 marked the issue as primary issue

[gzeoneth]

not quite sure, need further review

[livingrockrises]

imo validateSignature would have failed prior to getting this stage! can you showcase this with proof of test case?

[livingrockrises]

to replay the second op (after the wallet is already deployed) nonce is zero but you will also need to send initcode exactly the same! (when it is 0x for first op) and the userOp signature is signed over the whole struct so it will either fail with signature mismatch, or createSenderIfNeeded will revert (if you send same signature and initcode!)

[c4-sponsor]

livingrockrises marked the issue as sponsor disputed

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[c4-sponsor]

livingrockrises requested judge review

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Insufficient proof