[Medium-2] Sponsored transactions can be abused for profit

[code423n4]

Lines of code

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

Vulnerability details

Impact

When a user executes a transaction with their SmartAccount, the function execTransaction() can be used. In some cases, some protocols want to get promoted and will sponsorize users of the abstract wallet transactions by refunding them back their gas usage in their protocol tokens.

This function starts by calculating a startGas. To do so, it starts by making an estimate of the price of the transaction with the "base gas" (21000), and then add the calldata cost.

From the 27th page of the Ethereum yellow paper, The cost for 0 byte values in the calldata is 4 units of gas, and the one for non-zero bytes is 16:

In the current implementation, an estimate of 8 was done:

// initial gas = 21k + non_zero_bytes * 16 + zero_bytes * 4
//            ~= 21k + calldata.length * [1/3 * 16 + 2/3 * 4]
uint256 startGas = gasleft() + 21000 + msg.data.length * 8;

This appears to be wrong because there is no way to predict how much 0 bytes will a user input as the calldata of this function, which is the same for non-zero bytes in the calldata.

This means that sending a transaction with a calldata that is filled with 0 is going to cause startGas to overestimate the price of the calldata, and is going to send too much tokens from the sponsor.

Sponsors probably want their tokens distributed in a fair way, and not being abused by malicious users crafting calldata transaction filled with 0.

Proof of Concept

See the following gist which a test case for abusing of this: https://gist.github.com/iFrostizz/b075edb97b39eaac333315ee927f5d14 Please note that due to foundry limitations (because of the gas limit, while creating the calldata filled with 0), I could not clearly show how much tokens could be stolen. But the trick consists of filling the Transaction data field with a maximum of 0's.

Tools Used

Manual inspection

Recommended Mitigation Steps

Make a calculation on the lower end, by assuming that the calldata is only filled with 0 (even when not) to avoid giving away too much tokens from the sponsor.

function execTransaction(
    Transaction memory _tx,
    uint256 batchId,
    FeeRefund memory refundInfo,
    bytes memory signatures
) public payable virtual override returns (bool success) {
        uint256 startGas = gasleft() + 21000 + msg.data.length * 4;
        /* Snip */
}

[c4-judge]

gzeon-c4 marked the issue as primary issue

[gzeoneth]

This can only be abused by the smart contract owner, who sign the transaction, and also pay the gas refund. Relayer cannot abuse this as described and I think it is reasonable to assume 8 gas per bytes on mixed bytes.

[livingrockrises]

misconception and lack of proof. there are other reported issues which expand on this scenario but i will mark this invalid

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[c4-sponsor]

livingrockrises marked the issue as sponsor disputed

[livingrockrises]

496 is not duplicate of this issue

[livingrockrises]

476 is not a duplicate of this issue

[livingrockrises]

371 and #373 are not duplicates of this issue.

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Insufficient proof