search

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

2 stars 1 forks source link

Recovererc20 uses transfer -> token transfers do not verify that the tokens were successfully transferred (safeTransfer) #399

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

Impact

Some tokens do not revert the transaction when the transfer function fails or return false. Which requires us to check the return value after calling the transfer function.

Given that recoverERC20 can accept any tokens. A token such as ZRX would not revert leading to a loss of funds with that token.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Recommended Mitigation Steps

Use ERC20 safeTransfer when sending tokens. To allow a revert and prevent loss tokens. Further a Malicious Owner can send a malicious ERC20 Token into the contract

require(IERC20(tokenAddress).safeTransfer(owner, tokenAmount), "recoverERC20: Transfer failed");

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

Duplicate of #18