FETH.sol is susceptible to ERC20 approve functionality front-running exploit

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/FETH.sol#L234

Vulnerability details

Proof of concept

The problem is perfectly described here https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit

The tldr; is that if you approved a person to spend 100 tokens and then you want to decrease his allowance to 50, if he spends his 100 tokens allowance before you set his allowance to 50 he will be able to spend 50 more. This results in him being able to spend 150 tokens instead of your desired 50 allowance.

Impact

This can leads to much more FETH being transferred from a user’s wallet than what he wants, since if he is decreasing the allowance he would want less tokens to be spent from his wallet, but this way actually more tokens could be spent.

Recommendation

Add OpenZeppelin’s safeIncreaseAllowance and safeDecreaseAllowance functionality to FETH.sol. You can use this SafeERC20.sol implementation as a reference https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3dac7bbed7b4c0dbf504180c33e8ed8e350b93eb/contracts/token/ERC20/utils/SafeERC20.sol

[HardlyDifficult]

The FETH contract was listed as out of scope.

If it were in scope, this report would be valid. We had the same feedback in our last contest and plan to make the suggested change the next time we make improvements to the FETH contract.

[HickupHH3]

Agree with sponsor: https://github.com/code-423n4/2022-08-foundation/tree/792e00df429b0df9ee5d909a0a5a6e72bd07cf79#out-of-scope