code423n4 commented 2 years ago

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.

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

[H1] Owner of frxETHMinter can rug pull the contract #377

Lines of code

Vulnerability details

Impact

PoC

Recommended