Biconomy - Smart Contract Wallet contest - MalfurionWhitehat's results

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L424-L446 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L204-L220 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L239 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L264

Vulnerability details

Impact

The function SmartAccount.encodeTransactionData does not include the refundInfo.tokenGasPriceFactor parameter on the transaction hash that is signed by the wallet owner. Because of that, two different wallet transactions with different refund informations will be validated by SmartAccount.execTransaction's inner checkSignatures. As a result, an attacker can frontrun a valid transaction and submit a different refundInfo.tokenGasPriceFactor, that will be used on SmartAccount.handlePayment to calculate the payment. A malicious attacker can thus make the payment be higher than expected.

Proof of Concept

1. The user submits a valid transaction and refund info to the SmartAccount
2. Because refundInfo.tokenGasPriceFactor is not included on the hashed data, the transaction can be frontrun and this value altered
3. As a result, the payment amount will be higher than anticipated, which will lead to user loss of funds

Tools Used

Manual review

Recommended Mitigation Steps

Include refundInfo.tokenGasPriceFactor on the encoded transaction data. In addition, consider using EIP-712 for typed structured data hashing and signing.

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..ae18182 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -439,6 +439,7 @@ contract SmartAccount is
                     refundInfo.gasPrice,
                     refundInfo.gasToken,
                     refundInfo.refundReceiver,
+                    refundInfo.tokenGasPriceFactor,
                     _nonce
                 )
             );

Lines of code

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

Vulnerability details

Impact

The most recent canonical EntryPoint contract states that:

    /**
     * for simulation purposes, validateUserOp (and validatePaymasterUserOp) must return this value
     * in case of signature failure, instead of revert.
     */
    uint256 public constant SIG_VALIDATION_FAILED = 1;

This requirement is also described on the latest BaseAccount NatSpec, that should be inherited by SmartAccount.sol. The same goes for VerifyingPaymaster that is the sample code for VerifyingSingletonPaymaster.sol.

Proof of Concept

EntryPoint.simulateHandleOp expects SIG_VALIDATION_FAILED instead of a revert.

Tools Used

Manual review

Recommended Mitigation Steps

Update the codebase to use the latest AA source code from the repository eth-infinitism/account-abstraction repository. In addition, keep close attention to protocol changes through the developers' social media.

1. Use block.chainid on SmartAccount.getChainId

The use of inline assembly is unnecessary here:

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..928c16a 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -138,12 +138,7 @@ contract SmartAccount is
 
     /// @dev Returns the chain id used by this contract.
     function getChainId() public view returns (uint256) {
-        uint256 id;
-        // solhint-disable-next-line no-inline-assembly
-        assembly {
-            id := chainid()
-        }
-        return id;
+        return block.chainid;
     }
 
     //@review getNonce specific to EntryPoint requirements

2. Do not use tx.origin for testing purposes on SmartAccount._validateSignature. Instead, create a mock implementation that overrides this method and bypasses any necessary signature validation.

Mixing testing and deployment code will unnecessarily introduce dead code on production contracts and will make hinder maintainability.

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..78e960b 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -508,7 +508,7 @@ contract SmartAccount is
         bytes32 hash = userOpHash.toEthSignedMessageHash();
         //ignore signature mismatch of from==ZERO_ADDRESS (for eth_callUserOp validation purposes)
         // solhint-disable-next-line avoid-tx-origin
-        require(owner == hash.recover(userOp.signature) || tx.origin == address(0), "account: wrong signature");
+        require(owner == hash.recover(userOp.signature), "account: wrong signature");
         return 0;
     }
 

3. Emit events on important state variable changing operations: VerifyingSingletonPaymaster.setSigner

diff --git a/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol b/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
index 7716a01..6534bda 100644
--- a/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
@@ -28,6 +28,8 @@ contract VerifyingSingletonPaymaster is BasePaymaster {
     using PaymasterHelpers for bytes;
     using PaymasterHelpers for PaymasterData;
 
+    event VerifyingSignerChanged(address indexed oldVerifyingSigner, address indexed newVerifyingSigner);
+
     mapping(address => uint256) public paymasterIdBalances;
 
     address public verifyingSigner;
@@ -64,6 +66,7 @@ contract VerifyingSingletonPaymaster is BasePaymaster {
     */
     function setSigner( address _newVerifyingSigner) external onlyOwner{
         require(_newVerifyingSigner != address(0), "VerifyingPaymaster: new signer can not be zero address");
+        emit VerifyingSignerChanged(verifyingSigner, _newVerifyingSigner);
         verifyingSigner = _newVerifyingSigner;
     }
 

4. Enforce consistent error messages throughout the code

Because this project is heavily influenced by the eth-infinitism/account-abstraction's sample code, additional error messages included by the Biconomy project are inconsistent with existing ones. It is recommended to refactor them in order to keep the codebase consistent:

diff --git a/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol b/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
index 7716a01..fb5e334 100644
--- a/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
@@ -39,22 +39,22 @@ contract VerifyingSingletonPaymaster is BasePaymaster {
     }
 
     function deposit() public virtual override payable {
-        revert("Deposit must be for a paymasterId. Use depositFor");
+        revert("VerifyingPaymaster: deposit must be for a paymasterId. Use depositFor");
     }
 
     /**
      * add a deposit for this paymaster and given paymasterId (Dapp Depositor address), used for paying for transaction fees
      */
     function depositFor(address paymasterId) public payable {
-        require(!Address.isContract(paymasterId), "Paymaster Id can not be smart contract address");
-        require(paymasterId != address(0), "Paymaster Id can not be zero address");
+        require(!Address.isContract(paymasterId), "VerifyingPaymaster: paymaster Id can not be smart contract address");
+        require(paymasterId != address(0), "VerifyingPaymaster: paymaster Id can not be zero address");
         paymasterIdBalances[paymasterId] += msg.value;
         entryPoint.depositTo{value : msg.value}(address(this));
     }
 
     function withdrawTo(address payable withdrawAddress, uint256 amount) public override {
         uint256 currentBalance = paymasterIdBalances[msg.sender];
-        require(amount <= currentBalance, "Insufficient amount to withdraw");
+        require(amount <= currentBalance, "VerifyingPaymaster: insufficient amount to withdraw");
         paymasterIdBalances[msg.sender] -= amount;
         entryPoint.withdrawTo(withdrawAddress, amount);
     }
@@ -106,7 +106,7 @@ contract VerifyingSingletonPaymaster is BasePaymaster {
         // we only "require" it here so that the revert reason on invalid signature will be of "VerifyingPaymaster", and not "ECDSA"
         require(sigLength == 64 || sigLength == 65, "VerifyingPaymaster: invalid signature length in paymasterAndData");
         require(verifyingSigner == hash.toEthSignedMessageHash().recover(paymasterData.signature), "VerifyingPaymaster: wrong signature");
-        require(requiredPreFund <= paymasterIdBalances[paymasterData.paymasterId], "Insufficient balance for paymaster id");
+        require(requiredPreFund <= paymasterIdBalances[paymasterData.paymasterId], "VerifyingPaymaster: insufficient balance for paymaster id");
         return (userOp.paymasterContext(paymasterData), 0);
     }
 

5. Remove misleading comments

Because this project is heavily influenced by the eth-infinitism/account-abstraction's sample code, some comments are left out even after code changes by the Biconomy project. As a result, they are misleading with regard to the implementation. It is recommended to refactor them in order to keep the codebase easy to understand:

diff --git a/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol b/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
index 7716a01..15ff91e 100644
--- a/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol
@@ -122,7 +122,6 @@ contract VerifyingSingletonPaymaster is BasePaymaster {
     uint256 actualGasCost
   ) internal virtual override {
     (mode);
-    // (mode,context,actualGasCost); // unused params
     PaymasterContext memory data = context.decodePaymasterContext();
     address extractedPaymasterId = data.paymasterId;
     paymasterIdBalances[extractedPaymasterId] -= actualGasCost;

6. Remove unused/outdated code comments, unused variables, TODOs, commented out tests, and improve project documentation.

Currently, the project seems to be in active development and not in a final stage. Many places have unused/outdated code comments, unused variables, TODOs, and removed tests. This prevents developers and auditors from finding nuanced vulnerabilities, as more time is spent with minor issues. It is advised to polish the work-in-progress before deploying it to mainnet.