Inaccurate use of the gasleft() estimation

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L200
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L224
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L279
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L291
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L371
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L372
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L49
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L55
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L169
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/aa-4337/core/EntryPoint.sol#L186

Vulnerability details

Impact

The gasleft() check is used extensively in the codebase.

however the actual amount of gas left during the call is less than that (mainly due to the 1/64 rule, see below)

Reference for the 1/64 rule - EIP-150.

https://github.com/ethereum/EIPs/blob/master/EIPS/eip-150.md

Also check out evm.codes

https://www.evm.codes/?fork=merge

We have to formulate the impact of using gasleft() case by case starting from the execTransaction in SmartAccount

// 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;
//console.log("init %s", 21000 + msg.data.length * 8);
bytes32 txHash;
// Use scope here to limit variable lifetime and prevent `stack too deep` errors
{
	bytes memory txHashData =
		encodeTransactionData(
			// Transaction info
			_tx,
			// Payment info
			refundInfo,
			// Signature info
			nonces[batchId]
		);
	// Increase nonce and execute transaction.
	// Default space aka batchId is 0
	nonces[batchId]++;
	txHash = keccak256(txHashData);
	checkSignatures(txHash, txHashData, signatures);
}


// We require some gas to emit the events (at least 2500) after the execution and some to perform code until the execution (500)
// We also include the 1/64 in the check that is not send along with a call to counteract potential shortings because of EIP-150
require(gasleft() >= max((_tx.targetTxGas * 64) / 63,_tx.targetTxGas + 2500) + 500, "BSA010");

If the code above, the actual amount of gas left is less than gasleft(),

then the code below pass but the transaction can still run out of gas.

require(gasleft() >= max((_tx.targetTxGas * 64) / 63,_tx.targetTxGas + 2500) + 500, "BSA010");

When handling the gas price refund, gasleft() is also used:

if (refundInfo.gasPrice > 0) {
	//console.log("sent %s", startGas - gasleft());
	// extraGas = gasleft();
	payment = handlePayment(startGas - gasleft(), refundInfo.baseGas, refundInfo.gasPrice, refundInfo.tokenGasPriceFactor, refundInfo.gasToken, refundInfo.refundReceiver);
	emit WalletHandlePayment(txHash, payment);
}

the actual amount of gas left is less than gasleft(), the impact is that the receiver is not able to get the full gas refund payment.

gasleft() is also used when estimaing the gas cost in SmartWallet

function handlePaymentRevert(
	uint256 gasUsed,
	uint256 baseGas,
	uint256 gasPrice,
	uint256 tokenGasPriceFactor,
	address gasToken,
	address payable refundReceiver
) external returns (uint256 payment) {
	uint256 startGas = gasleft();
	// solhint-disable-next-line avoid-tx-origin
	address payable receiver = refundReceiver == address(0) ? payable(tx.origin) : refundReceiver;
	if (gasToken == address(0)) {
		// For ETH we will only adjust the gas price to not be higher than the actual used gas price
		payment = (gasUsed + baseGas) * (gasPrice < tx.gasprice ? gasPrice : tx.gasprice);
		(bool success,) = receiver.call{value: payment}("");
		require(success, "BSA011");
	} else {
		payment = (gasUsed + baseGas) * (gasPrice) / (tokenGasPriceFactor);
		require(transferToken(gasToken, receiver, payment), "BSA012");
	}
	uint256 requiredGas = startGas - gasleft();
	//console.log("hpr %s", requiredGas);
	// Convert response to string and return via error message
	revert(string(abi.encodePacked(requiredGas)));
}

the actual amount of gas left is less than gasleft(), this leads to inaccurate estimation of the gas needed to in order to complete the transaction.

The gasleft() is also used extensively in EntryPoint.sol

/**
 * execute a user op
 * @param opIndex into into the opInfo array
 * @param userOp the userOp to execute
 * @param opInfo the opInfo filled by validatePrepayment for this userOp.
 * @return collected the total amount this userOp paid.
 */
function _executeUserOp(uint256 opIndex, UserOperation calldata userOp, UserOpInfo memory opInfo) private returns (uint256 collected) {
	uint256 preGas = gasleft();
	bytes memory context = getMemoryBytesFromOffset(opInfo.contextOffset);

	try this.innerHandleOp(userOp.callData, opInfo, context) returns (uint256 _actualGasCost) {
		collected = _actualGasCost;
	} catch {
		uint256 actualGas = preGas - gasleft() + opInfo.preOpGas;
		collected = _handlePostOp(opIndex, IPaymaster.PostOpMode.postOpReverted, opInfo, context, actualGas);
	}
}

the actual amount of gas left is less than gasleft(), transaction can run out of gas unexpectedly.

Proof of Concept

Besides using a few units of gas between the check and the actual call, there's also a rule that only 63/64 of the remaining gas would be dedicated to an (external) function call. Since there are 2 external function calls done (nonRevertingBridgeCall() and the actual call to the bridge) ~2/64 of the gas isn't sent to the bridge call and can be used after the bridge call runs out of gas.

The following PoC shows that if the amount of gas left before the call is at least 1 million then the execution can continue after the bridge call fails

(note: there is no bridge in the context of the smart wallet, this POC just shows that the using gasleft() is not a accurate way to estimate the gas left)

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "forge-std/Test.sol";

contract ContractTest is Test {
    event FailedOperatorJob(bytes32 jobHash);
    uint256 private _inboundMessageCounter;
    mapping(bytes32 => bool) private _failedJobs;
    constructor(){
        _inboundMessageCounter = 5;
    }
    function testGas64() public {
        this.entryPoint{gas:1000000}();
    }

    Bridge bridge = new Bridge();
    event GasLeftAfterFail(uint left);

    function entryPoint() public {

        console2.log("Gas left before call: ", gasleft());

        bytes32 hash = 0x987744358512a04274ccfb3d9649da3c116cd6b19c535e633ef8529a80cb06a0;

        try this.intermediate(){
        }catch{
            // check out how much gas is left after the call to the bridge failed
            console2.log("Gas left after failure: ", gasleft());
            // simulate operations done after failure
            _failedJobs[hash] = true;
            emit FailedOperatorJob(hash);
        }
        ++_inboundMessageCounter;
        console2.log("Gas left at end: ", gasleft());

    }

    function intermediate() public{
        bridge.bridgeCall();
    }
}


contract Bridge{
    event Done(uint gasLeft);

    uint256[] myArr;

    function bridgeCall() public {
        for(uint i =1; i <= 100; i++){
            myArr.push(i);
        }
        // this line would never be reached, we'll be out of gas beforehand
        emit Done(gasleft());
    }
}

Output of POC:

Gas left before call:  999772
Gas left after failure:  30672
Gas left at end:  1628

Tools Used

Foundry, hardhat, and manual review.

Recommended Mitigation Steps

Modify the required amount of gas left to gasLimit + any amount of gas spent before reaching the call(), then multiply it by 32/30 to mitigate the 1/64 rule (+ some margin of safety maybe).

Or In fact I think the gas estimation can be done in off-chain manner.

[c4-judge]

gzeon-c4 marked the issue as primary issue

[gzeoneth]

Risk seems low here with proper gas estimation to set the limit.

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[livingrockrises]

none of issues are duplicates of this issue.

[c4-sponsor]

livingrockrises requested judge review

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)

[c4-judge]

gzeon-c4 marked the issue as grade-b