Frontrunning Vulnerability in approve() function of ERC20

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L131-L137

Vulnerability details

Impact

The approve() function in the provided ERC20 contract is susceptible to a frontrunning attack. Malicious actors can exploit the timing between an approve() transaction being sent and it getting mined, potentially allowing them to spend more tokens than the token owner intended. If an owner tries to change the allowance of a spender, the spender can spend the original allowance before the new one takes effect, and then spend the new allowance, resulting in a total expenditure exceeding the latest set amount.

Proof of Concept

Consider the following scenario:

1. Token owner Alice has approved spender Bob to withdraw 1,000 tokens.

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L131-L137

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

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

        return true;
    }

2. Alice decides to modify this approval to allow Bob to withdraw only 1,500 tokens. She sends a transaction to set this new allowance.
3. Bob, monitoring the transaction pool, sees Alice's pending transaction.
4. Before Alice's transaction is mined, Bob sends a transaction (with a higher gas price for priority) to withdraw the initial 1,000 tokens.
5. If Bob's transaction is mined before Alice's, he will have withdrawn the 1,000 tokens.
6. Once Alice's transaction is mined, the allowance is reset to 1,500 tokens, allowing Bob to withdraw an additional 1,500 tokens.
7. In total, Bob can withdraw 2,500 tokens, even though Alice intended a maximum of 1,500 tokens in the second approval.

Tools Used

Manual

Recommended Mitigation Steps

Before increasing or decreasing the allowance, the contract could require that the current allowance be set to 0. This way, Alice would have to first send a transaction setting the allowance to 0 before setting it to a new value, ensuring Bob cannot front-run in between. The downside is that this requires two separate transactions, which might not be very user-friendly.

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/token/ERC20.sol#L131-L137

    function approve(address spender, uint256 value) external returns (bool) {
+        require((allowance[msg.sender][spender] == 0), "ERC20: set allowance to 0 first");
        allowance[_msgSender()][spender] = value;

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

        return true;
    }

Alternatively, simply remove approve() since users can always use increaseAllowance() for the same intended purpose.

In your contract, you've provided the increaseAllowance() and decreaseAllowance() functions, which is good. Informing users to prefer these methods over a direct approve() for changing allowances can mitigate the front-running risk.

Assessed type

Timing

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #145

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory:
Invalid