Frax Ether Liquid Staking contest - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

frxETHMinter.recoverEther function sends all contract funds to the owner. I has onlyByOwnGov modifier that allows it to be called by owner or timelock contract.

In case where the admin wallet has been hacked, the attacker can drain all funds. Or malicious owner can also send all funds to it.

I would recommend to allow to call that function only for timelock through the governance. Also i would change the function to take an address param as a sender to not send funds to owner only(in case when owner wallet is hacked for example).

Recommended Mitigation Steps

Remove ability for the owner to send all funds to himself.

Lines of code

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

Vulnerability details

Impact

frxETH is going to be stable coin to eth that means that frxETH should be backed 1:1 with ether. frxETH is minted by frxETHMinter using _submit function. It can be triggered by receive function as well (that's important here).

frxETHMinter.moveWithheldETH function allows to send some amount of currentWithheldETH and then decrease the value that you can withdraw here and sends the value to the provided address to.

If this function will be called(by mistake or no) with param to == address(frxETHMinter), then some amount of ether will be sent to frxETHMinter, that will trigger receive function and then more amount of frxETH token will be minted and frxETH token is not pegged to ether as 1:1 anymore as we minted new frxETH tokens without additional ether supply.

Proof of Concept

This test was ran in frxETHMinterTest. It shows that after withdrawing to minter new frxETH tokens were minted while balance of frxETHMinter remain the same.

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

        vm.deal(FRAX_COMPTROLLER, 32 ether);

        // Set the withhold ratio to 50% (5e5)
        minter.setWithholdRatio(500000);

        // Deposit 32 ETH for frxETH
        vm.expectEmit(true, true, false, true);
        emit TokenMinterMinted(address(minter), FRAX_COMPTROLLER, 32 ether);
        vm.expectEmit(true, true, false, true);
        emit ETHSubmitted(FRAX_COMPTROLLER, FRAX_COMPTROLLER, 32 ether, 16 ether);
        minter.submit{ value: 32 ether }();

        // Check that 16 ether was withheld
        assertEq(minter.currentWithheldETH(), 16 ether);

        //amount of frxToken before withdraw
        uint256 currentSupply = frxETHToken.totalSupply();
        uint256 currentMinterBalance = address(minter).balance;
        minter.moveWithheldETH(payable(minter), minter.currentWithheldETH());
        //note that also 50% were withheld
        assertEq(minter.currentWithheldETH(), 8 ether);
        //amount of frxToken after withdraw
        uint256 newSupply = frxETHToken.totalSupply();
        uint256 newMinterBalance = address(minter).balance;
        //we have minted new tokens
        assertTrue(newSupply > currentSupply);
        //balance of minter hasn't changed
        assertTrue(newMinterBalance == currentMinterBalance);
        vm.stopPrank();
    }

Tools Used

VsCode

Recommended Mitigation Steps

Make sure that to param is not the frxETHMinter contract.

1. Check address param for 0 address. https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L60-L62 https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L41 https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L34
2. Do not initialize variables with default values https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L63-L64 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L94
3. Change message here to Validator stack is not empty https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L137
4. Use require(!minters[minter_address], "Address already exists") instead. https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L68
5. Use require(minters[minter_address], "Address nonexistant") instead https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L78