Frax Ether Liquid Staking contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the frxETHMinter contract, the owner can call the recoverEther function to rug all the ETH in the contract.

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

        emit EmergencyEtherRecovered(amount);
    }

The owner can even set depositEtherPaused to false to disable deposit of ETH and thus rug more than 32 ETH.

    function togglePauseDepositEther() external onlyByOwnGov {
        depositEtherPaused = !depositEtherPaused;

        emit DepositEtherPaused(depositEtherPaused);
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider removing the recoverEther function

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L75-L87

Vulnerability details

Impact

In sfrxETH contracts, the result of previewMint() changes with the state of the contract, which causes the value of amount to be volatile in the mintWithSignature function when approveMax is false. And when using the mintWithSignature function, which requires the user to sign for an accurate amount value, when the amount used differs from the result of previewMint(), mintWithSignature will not work. Consider the following scenarios. User A signs using amount = 1000 and calls the mintWithSignature function. During execution, the previous transaction in the same block changes the state of the contract so that previewMint(shares) == 1001, so the transaction is reverted due to a signature check failure.

Proof of Concept

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L75-L87 https://github.com/transmissions11/solmate/blob/bff24e835192470ed38bf15dbed6084c2d723ace/src/mixins/ERC4626.sol#L140-L144

Tools Used

None

Recommended Mitigation Steps

Consider that in the mintWithSignature function, the user provides a maxAmount, and then requires maxAmount >= previewMint(shares) and uses maxAmoun to verify the signature