Unsigned `tokenGasPriceFactor` parameter

[code423n4]

Lines of code

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

Vulnerability details

Description

For the calculation of the amount of the token to be paid to the relayer tokenGasPriceFactor value is used. The corresponding logic is the following:

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

So, the number of tokens that the relayer should receive is inversely proportional to the value of this variable. But the tokenGasPriceFactor parameter is not signed by the owner:

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

Using this fact the relayer can pass any value of tokenGasPriceFactor parameter to receive a greater amount than the user expected to pay.

Attack scenario

The user sends a transaction to the relayer. By the formula, payment value should be equal gasUsed/tokenGasPriceFactor. The user expects that the tokenGasPriceFactor will be equal to 100 for the specified token, but the relayer put tokenGasPriceFactor to 1. Relayer receives 100 times greater payout.

Impact

Obtaining a greater relayer benefit than the user expects, theft of user funds.

Recommended Mitigation Steps

Add the refundInfo.tokenGasPriceFactor to the preimage of the tx hash which should be signed by the owner.

[c4-judge]

gzeon-c4 marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as selected for report

[c4-judge]

gzeon-c4 marked issue #123 as primary and marked this issue as a duplicate of 123