Frax Ether Liquid Staking contest - peritoflores's results

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L191 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L199

Vulnerability details

Impact

Owner of the contract is able to leave with all the tokens and ETH of the contract, which makes protocol trustless

PoC

You have implemented a function to

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

First of all this is a direct bypass of the

 function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov {
 require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");
currentWithheldETH -= amount;
(bool success,) = payable(to).call{ value: amount }("");
require(success, "Invalid transfer");
emit WithheldETHMoved(to, amount);
}

So in any case a bypass of this is just a high risk.

Moreover, it is not a good idea to have any kind of emergencyRecovery . It is correct to be sure that your protocol will never have ETH stucked by its logic. This is why we are here ! :). Otherwise anybody will implement the same solution and we will never be worried about stucked ETH.

A similar issue here

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

​ In other protocols this function generally is limited to all tokens except those handled by the protocol. You need to be sure that those ERC20 are not frxETHToken or sfrxToken.

Recommended

Remove recoverEther() function

And for recoverERC20() be sure to check that

[+]  require(tokenAddress != address(frxETHToken), "Some message")
[+]  require(tokenAddress != address(sfrxETHToken), "Some message")

Lines of code

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

Vulnerability details

Impact

Contract frxETHMinter is unable to recover tokens like USDT

PoC

Tokens that return void on transfer, that is, those who do not follow ERC20 standard will revert when you try to assign the output to a boolean variable. This is the case in you function recoverERC20

Recommended

I believe that it is better just to remove require and check balance off-chain. Otherwise, you will need libraries like SafeTransfer but I don't think that this is really neccessary. Tokens return false generally when you try to transfer more balance that you have or you are not approved.

[-]  require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");
[+]  IERC20(tokenAddress).transfer(owner, tokenAmount);

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L70

Vulnerability details

Impact

​ Risk of reentrancy in submitAndDeposit function.

PoC

​ I see that you added the non reentrant modifier to the internal function _submit().
The problem with this is that you are not protecting some parts of the function submitAndDeposit()

function submitAndDeposit(address recipient) external payable returns (uint256 shares) {
    // Give the frxETH to this contract after it is generated
    _submit(address(this));
    // Approve frxETH to sfrxETH for staking
    frxETHToken.approve(address(sfrxETHToken), msg.value);
    // Deposit the frxETH and give the generated sfrxETH to the final recipient
    uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);  
    require(sfrxeth_recieved > 0, 'No sfrxETH was returned');
    return sfrxeth_recieved;
}

In this case if sfrxETHToken.deposit(msg.value, recipient); it could reenter depending on implementation (actually we only have access to interface IsfrxETH)

In any case it is better to add reentrant modifier to every external function that you want to protect.

Recommended

Remove nonReentrant modifier from _submit() and add it to every function that uses it