Gas Refunds can be lost

Summary

Due to the usage of tx.origin when handling gas, refunds meant for a smart contract wallet will be lost.

Vulnerability Detail

There are two places where gas refunds are handled, either in native ETH:

    function _refundGas(uint256 startGas) internal {
        unchecked {
            uint256 balance = address(this).balance;
            if (balance == 0) {
                return;
            }
            uint256 basefee = min(block.basefee, MAX_REFUND_BASE_FEE);
            uint256 gasPrice = min(tx.gasprice, basefee + MAX_REFUND_PRIORITY_FEE);
            uint256 gasUsed = min(startGas - gasleft() + REFUND_BASE_GAS, MAX_REFUND_GAS_USED);
            uint256 refundAmount = min(gasPrice * gasUsed, balance);
            (bool refundSent, ) = tx.origin.call{ value: refundAmount }('');
            emit RefundableVote(tx.origin, refundAmount, refundSent);
        }
    }

or in ethToken:

    /// @dev refunds gas using the `ethToken` instead of ETH
    function refundGas(IERC20 ethToken, uint256 startGas) internal {
        unchecked {
            uint256 balance = ethToken.balanceOf(address(this));
            if (balance == 0) {
                return;
            }
            uint256 basefee = min(block.basefee, MAX_REFUND_BASE_FEE);
            uint256 gasPrice = min(tx.gasprice, basefee + MAX_REFUND_PRIORITY_FEE);
            uint256 gasUsed = startGas - gasleft() + REFUND_BASE_GAS;
            uint256 refundAmount = min(gasPrice * gasUsed, balance);
            ethToken.safeTransfer(tx.origin, refundAmount);
        }
    }

The problem here lays in the usage of tx.origin. There is a clear direction on the Ethereum mainnet to move from EOA's to Smart Contract Wallets. The project is aware of this because, for example, they use isValidSignatureNow:

    /**
     * @dev Checks if a signature is valid for a given signer and data hash. If the signer is a smart contract, the
     * signature is validated against that smart contract using ERC1271, otherwise it's validated using `ECDSA.recover`.
     *
     * NOTE: Unlike ECDSA signatures, contract signatures are revocable, and the outcome of this function can thus
     * change through time. It could return true at block N and false at block N+1 (or the opposite).
     */
    function isValidSignatureNow(
        address signer,
        bytes32 hash,
        bytes memory signature
    ) internal view returns (bool) {
        (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature);
        return
            (error == ECDSA.RecoverError.NoError && recovered == signer) ||
            isValidERC1271SignatureNow(signer, hash, signature);
    }

Which also provides the functionality for contract signatures to be validated.

However, in the case of the two examples of gas refunds, these get sent to the tx.origin. When a smart contract wallet interacts with this protocol, the smart contract wallet will not be the tx.origin due to the nature of Account Abstraction, which means the refund will go to the wrong address since tx.origin will not be the smart contract wallet.

Impact

This results in gas refund funds lost.

Code Snippet

https://github.com/sherlock-audit/2024-03-nouns-dao-2/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/libs/GasRefund.sol#L34-L46

Tool used

Manual Review

Recommendation

Handle refunds differently for smart contract wallets.

sherlock-audit / 2024-03-nouns-dao-2-judging

FassiSecurity - Gas Refunds can be lost #4