unRekt - Not taking in account `approve` race condition can lead to frontrunning attack

[sherlock-admin3]

unRekt

Medium

Not taking in account approve race condition can lead to frontrunning attack

Summary

Vulnerability Detail

Alice calls approve(Bob, 100), allocating 100 tokens for Bob to spend Alice opts to change the amount approved for Bob to spend to a lesser amount via approve(Bob, 50). Once mined, this decreases the number of tokens that Bob can spend to 50. Bob sees the transaction and calls transferFrom(Alice, X, 100) before approve(Bob, 50) has been mined. If Bob’s transaction is mined prior to Alice’s, 1000 tokens will be transferred by Bob. However, once Alice’s transaction is mined, Bob can call transferFrom(Alice, X, 50), transferring a total of 150 tokens despite Alice attempting to limit the total token allowance to 50.

Impact

It opens yourself and users of the token up to frontrunning

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L64

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L82

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/EtherFi.sol#L26

Tool used

Manual Review

Recommendation

A potential fix includes preventing a call to approve if all the previous tokens are not spent through adding a check that the allowed balance is 0 or consider using the safeIncreaseAllowance() / safeDecreaseAllowance() / forceApproval() pattern from OpenZeppelin/SafeERC20

If gas usage is a concern, use an approveOnlyIfBelowThreshold() || approveOnlyIfApprovalZero() pattern, in which approvals are only set if the approval falls below a predefined threshold, or zero.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Low/QA at most