Frax Ether Liquid Staking contest - pashov's results

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/8073cc4ca44d9162873494f1cd9915a5d7b46f2b/src/frxETHMinter.sol#L191

Vulnerability details

Proof of concept

In frxETHMinter.sol there is the following code

function recoverEther(uint256 amount) external onlyByOwnGov {
        (bool success,) = address(owner).call{ value: amount }("");
        require(success, "Invalid transfer");

        emit EmergencyEtherRecovered(amount);
    }

This allows the owner of the contract (bypassing timelock) to withdraw all of the submitted Ether to the contract before it was deposited to the ETH 2.0 deposit contract, esentially rugging everyone who submitted Ether to mint frxETH.

Impact

The impact of this issue is that since the protocol is ruggable, it’s reputation will suffer. Also if this is exploited then all of the users will essentially lose all of their submitted Ether

Recommendation

Remove the recoverEther functionality completely.

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/8073cc4ca44d9162873494f1cd9915a5d7b46f2b/src/ERC20/ERC20PermitPermissionedMint.sol#L76

Vulnerability details

Proof of concept

The code loops through the minters_array until it finds the minter_address to be removed. Every new minter will be pushed to the end of the minters_array array. In the case that there were many minters added already and an owner adds a wrong address as the minter it is possible that the for loop takes too much gas and the transaction reverts because it will hit the block gas limit.

Impact

The impact is a potential Denial of Service when trying to remove a wrong or a malicious minter.

Recommendation

Remove the minters_array since it’s not used for any critical logic in the code or instead implement the same mechanism to pop a minter from the end of the array as in OperatorRegistry.sol popValidators()

Lines of code

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

Vulnerability details

Proof of concept

The frxETHMinter.sol contract has the following code

function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyByOwnGov {
        require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");

        emit EmergencyERC20Recovered(tokenAddress, tokenAmount);
    }

but the way to check if the transfer is successful (with a require statement) won’t work for non-ERC20 compliant tokens like USDT because they do not return a boolean value on transfer(), so the require statement will always revert.

Impact

The impact is that the implemented recoverERC20 functionality won’t work as expected, resulting in such Non-ERC20 compliant tokens like USDT getting stuck in the contract

Recommendation

Use OpenZeppelin’s SafeERC20 library - safeTransfer & safeTransferFrom