PRAISE - Spender can frontrun setApprovalForERC20() in FootiumEscrow.sol

[sherlock-admin]

PRAISE

high

Spender can frontrun setApprovalForERC20() in FootiumEscrow.sol

Summary

The approve function is frontrunnable.

Vulnerability Detail

Lets say Alice is the owner of these tokens and Bob is the spender:

• Now Alice approves Bob to spend his tokens by passing in Bob's address
• she approves him to transfer 1000 tokens.
• After sometime she decide to change from 1000 tokens to 500 tokens
• Bob notices the Alice's second transaction before it was mined and quickly sends another transaction that calls the .transferFrom method to transfer 1000 tokens from Alice somewhere
• If the Bob's transaction will be executed before the Alice's transaction, then Bob will successfully transfer 1000 tokens from Alice and will gain an ability to transfer another 500 tokens too
• Before Alice noticed that something went wrong, Bob calls the .transferFrom method again, this time to transfer 500 tokens from Alice.
• So, Alice's attempt to change the Bob's allowance from 1000 to 500 made it possible for Bob to transfer 1000+500 of Alice's tokens, while Alice never wanted to allow so many of her tokens to be transferred by Bob.

Impact

It can result in losing tokens when club owner approves ERC20 tokens to any malicious Spender.

Code Snippet

    function setApprovalForERC20(
        IERC20 erc20Contract,
        address to,
        uint256 amount
    ) external onlyClubOwner {
        erc20Contract.approve(to, amount);
    }

https://github.com/sherlock-audit/2023-04-footium/blob/main/footium-eth-shareable/contracts/FootiumEscrow.sol#L75-L81

Tool used

Manual Review

Recommendation

Use increaseAllowance and decreaseAllowance instead of approve as OpenZeppelin ERC20 implementation. Please see details here:

https://forum.openzeppelin.com/t/explain-the-practical-use-of-increaseallowance-and-decreaseallowance-functions-on-erc20/15103/4

[logiclogue]

This is a duplicate for another issue in this list. However, this one has a better suggestion

[GalloDaSballo]

Escalate for 10 USDC

The finding says that the allowance can be front-run, but it fails to acknowledge the fact that if the spender didn't want to allow the extra allowance to be at risk, they would have sent a approve(0) first in a separate transaction

The finding is also logically equivalent to saying that granting allowance to a scammer is a vulnerability, which would be considered out of scope / invalid

I would recommend closing as invalid

If you believe that the allowance is a honey-pot vector then you should merge it with: #291

[sherlock-admin]

Escalate for 10 USDC

The finding says that the allowance can be front-run, but it fails to acknowledge the fact that if the spender didn't want to allow the extra allowance to be at risk, they would have sent a approve(0) first in a separate transaction

The finding is also logically equivalent to saying that granting allowance to a scammer is a vulnerability, which would be considered out of scope / invalid

I would recommend closing as invalid

If you believe that the allowance is a honey-pot vector then you should merge it with: #291

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[securitygrid]

This question really doesn't matter much. Can't reach M, suggest low/info.

[ctf-sec]

Agree with GalloDaSballo, can be a valid low / informational

[hrishibhat]

Escalation accepted

Valid low
Based on the escalation and other comments this issue can be considered as low/informational.

[sherlock-admin]

Escalation accepted

Valid low
Based on the escalation and other comments this issue can be considered as low/informational.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.