search

code-423n4 / 2022-09-frax-findings

2 stars 1 forks source link

Missing checks of return value of transfer() #327

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L200

Vulnerability details

Bug founded in frxETHMinter.sol

Missing checks of return value of transfer()

Brief Detail

Some implementations of ERC20 tokens do not revert during failure - they only return false. Callers of transfer() must check the return code to see whether the transfer was successful or not.

Impact

The code as currently implemented does not handle these sorts of tokens properly when they're a pool's reward token, which would lead to the loss of treasury and reward pool funds.

Proof Of Concept

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L200

src/frxETHMinter.sol:200:        require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20's safeTransfer() instead.

FortisFortuna commented 2 years ago

Not really medium risk. Technically you could use safeTransfer, but if someone were to accidentally send something to this contract, it would most likely be either ETH, FRAX, frxETH, or sfrxETH, all of which are transfer compliant.

joestakey commented 2 years ago

Invalid, the call is wrapped in a require statement