Frontrunning Approve Race Condition

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L131
https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L139
https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/token/ERC20.sol#L148

Vulnerability details

Vulnerability Details

There are no checks in the already approved spender function, which may lead to frontrunning.

Ethereum mempool is the place where all pending transactions sit until miner decided to include them into the block. For the most time, transactions with the highest gas prices are included first as the miners get the gas price for including them into the block.

Some have found a way to exploit this miners behaviour. Front-running, works like this: Transaction A is broadcasted with a higher gas price than an already pending transaction B so that A gets mined before B.

Exploit Scenario

If Bob monitors the mempool, he could submit his transaction with a higher gas cost so it would be executed before Alice's TX.

Attack is also possible because approve() overrides current allowance. It doesn't increase/decrease allowance.

1. Alice allows Bob to transfer 20 of Alice's tokens by calling approve method on Token smart contract passing Bob's address and 20 as method arguments. approve("0xBob", 20);
2. After some time, Alice decides to change from 20 to 10 the number of Alice's tokens Bob is allowed to transfer, so she calls approve method again, this time passing Bob's address and 10 as method arguments. approve("0xBob", 10);
3. Bob notices Alice's second transaction before it was mined and quickly sends another transaction that calls transferFrom method to transfer 20 Alice's tokens to himself. transferFrom("0xAlice", "0xBob", 20);
4. If Bob's transaction will be executed before Alice's transaction, Bob will successfully transfer 20 Alice's tokens and gain the ability to transfer another 10 tokens.
5. Before Alice noticed that something went wrong, Bob calls transferFrom method again, this time to transfer 10 Alice's tokens. This results in Bob acquiring 30 Alice's tokens instead of only 10.

Impact

Unsafe ERC20 approve that do not handle non-standard erc20 behavior.
1.Some token contracts do not return any value.
2.Some token contracts revert the transaction when the allowance is not zero.

Proof of Concept

    function approve(address spender, uint256 value) external returns (bool) {
        allowance[_msgSender()][spender] = value;

        emit Approval(_msgSender(), spender, value);

        return true;
    }

    function increaseAllowance(address spender, uint256 addedValue) external returns (bool) {
        uint256 newValue = allowance[_msgSender()][spender] + addedValue;
        allowance[_msgSender()][spender] = newValue;

        emit Approval(_msgSender(), spender, newValue);

        return true;
    }

    function decreaseAllowance(address spender, uint256 subtractedValue) external returns (bool) {
        uint256 allowed = allowance[_msgSender()][spender];
        require(allowed >= subtractedValue, "ERC20/insufficient-allowance");
        unchecked {
            allowed = allowed - subtractedValue;
        }
        allowance[_msgSender()][spender] = allowed;

        emit Approval(_msgSender(), spender, allowed);

        return true;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

First: To address this issue, Alice must initiate two transactions. First, she sets the allowance to 0, effectively resetting it for Bob. The second transaction is required to approve the new desired allowance.

Second: Employ non-standard functions provided by ERC20, namely increaseAllowance() and decreaseAllowance(). So if the Bob was approved before, and the approved amount is higher/lower, call the increaseAllowance() or decreaseAllowance(). These functions are integral components of the widely adopted OpenZeppelin ERC20 implementation but the problem is it's possible to have human error calling the approve function instead of increaseAllowance() and decreaseAllowance().

Assessed type

ERC20

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

hieronx marked the issue as disagree with severity

[hieronx]

increase/decreaseAllowance were removed from OZ recently for a reason: OpenZeppelin/openzeppelin-contracts#4585 This has never been exploited in production. I would consider this low.

[c4-sponsor]

hieronx (sponsor) acknowledged

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Overinflated severity

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid