`recoverEther` not updating `currentWithheldETH` breaks calculation of withheld amount for further deposits

[code423n4]

Lines of code

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

Vulnerability details

The emergency exit function recoverEther allows the owner to retrieve the ETH in case an issue were to happen.

The problem is that this function does not update currentWithheldETH.

This means upon deposit starting again after the emergency recovery, currentWithheldETH will have an offset and will not match the withholdRatio.

Direct consequences:

• depositEther may not deposit the expected amount of ETH into the ETH 2.0 staking contract.
• the amount of ETH moved to an external yield protocol using moveWithheldETH() will be higher than what it should be.

Impact

Medium

Proof Of Concept

• frxETHMinter has a withholdRatio set to 2 * 1e5 - ie the contract is meant to hold 20% of the ETH deposited.
• users deposit ETH into the contract.
• An issue happens and the owner calls recoverEther(address(this).balance). Before the call, the total balance was 1e20 (100 ETH), and currentWithheldETH == 2 * 1e19 - for simplicity we assume no calls to moveWithheldETH or depositEther have been done yet.
• the ETH balance of the minter is now 0, but currentWithheldETH is still 2 * 1e19
• users start depositing again.
• At this point, the total balance is now 1e20 (100 ETH), and currentWithheldETH == 4 * e19
• The owner calling depositEther deposits 32 ether instead of 64 ether, because currentWithheldETH == 40 ether instead of 20 ether. The owner can also call moveWithheldETH with amount == 4 * 1e19 instead of amount == 2 * 1e19.

You can add the following foundry test in frxETHMinter.t.sol to reproduce the issue:

• first declare address Alice = address(1); before the setUp()

function testIssueRecoverEther() public {
        vm.startPrank(FRAX_COMPTROLLER);

        // Note the starting ETH balance of the comptroller
        uint256 starting_eth = FRAX_COMPTROLLER.balance;

        // Give Alice 200 eth
        vm.deal(Alice, 200 ether);
        // Set the withhold ratio to 20% (2 * 1e5)
        minter.setWithholdRatio(200000);
        vm.stopPrank();

        vm.startPrank(Alice);

        //deposit 100 ETH
        minter.submit{ value: 100 ether }();
        vm.stopPrank();

        vm.startPrank(FRAX_COMPTROLLER);
        // Recover all
        minter.recoverEther(100 ether);

        // Make sure the FRAX_COMPTROLLER got 100 ether back
        assertEq(FRAX_COMPTROLLER.balance, starting_eth + (100 ether));

        //check `currentWithheldETH`: it has not been reset and is still 20 ETH
        assertEq(minter.currentWithheldETH(), 20 ether);
        vm.stopPrank();

        vm.startPrank(Alice);
        //deposit 100 ETH
        minter.submit{ value: 100 ether }();
        //check `currentWithheldETH`: because of the offset, it is now 40 ETH, ie 40% of the total ETH in the minter
        assertEq(minter.currentWithheldETH(), 40 ether);
        assertEq(address(minter).balance, 100 ether);
        vm.stopPrank();

        vm.startPrank(FRAX_COMPTROLLER);
        //Owner can call moveWithheldETH, transferring more than 40% of the balance, while the withheld amount should be 20%
        minter.moveWithheldETH(payable(address(Alice)), 40 ether);
        assertEq(address(minter).balance, 60 ether);
        vm.stopPrank();
    }

Tools Used

Manual Analysis, Foundry

Mitigation

Update currentWithheldETH in recoverEther :

+            currentWithheldETH = currentWithheldETH >= amount ? currentWithheldETH - amount : 0 ;
192:         (bool success,) = address(owner).call{ value: amount }("");
193:         require(success, "Invalid transfer");
194: 
195:         emit EmergencyEtherRecovered(amount);

[FortisFortuna]

@denett withholdRatio is is not an iron rule and can be updated by the owner at will. recoverEther will likely only be used when we are migrating to a new minting contract, so the accounting in that case is no longer important.

[0xean]

346 has some great suggestion in it on ensuring user safety in an emergency scenario and think that both of these issues do highlight a valid concern that ultimately could affect the protocol in an emergency scenario.