code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L166-L174 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L191-L196

Vulnerability details

Impact

In the frxETHMinter contract both the owner and governance timelock have the power to call the functions moveWithheldETH and recoverEther, those functions allow the transfer of the ETH from frxETHMinter to the owner or a given account, this means that the owner can easily call one of those functions to withdraw all the ETH balance and run with it which is basically a Rug Pull.

The impact of this is the following :

For the moveWithheldETH function the impact is lower as even if the owner rug pull the funds only the currentWithheldETH will be transferred which will of course remove the market liquidity but the other ETH funds intended for staking will be safe.
In the case of the recoverEther function the impact is much higher as the owner can call this function at any time and withdraw the full frxETHMinter contract ETH balance this will stop completely the staking process as there will be no ETH to be deposited and the users who held sfrxETHToken will not receive any rewards and won't be able to get their ETH back.

Proof of Concept

Both moveWithheldETH and recoverEther functions have the onlyByOwnGov modifier which means that they can be called either by the governance timelock or the owner at any time :

function moveWithheldETH

    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);
    }

function recoverEther

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

        emit EmergencyEtherRecovered(amount);
    }

Tools Used

Visual audit

Recommended Mitigation Steps

There are two solution to avoid the risk of a Rug pull :

The receiver of ETH funds in both moveWithheldETH and recoverEther functions should be set to the protocol treasury so even if the functions are called by the owner the ETH will be transferred to the treasury and not to another address.
The frxETHMinter contract should only allow the governance timelock to call the functions moveWithheldETH and recoverEther, this can be done by replacing the onlyByOwnGov modifier with an onlyByGov modifier.

FortisFortuna commented 2 years ago

We are well aware of the permission structure. The owner will most likely be a large multisig. We mentioned the Frax Multisig in the scope too.

joestakey commented 2 years ago

Duplicate of #107

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