Frax Ether Liquid Staking contest - neko_nyaa's results

Lines of code

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

Vulnerability details

Proof of Concept

The function recoverEther() does not have any checks to ensure the total supply of frxETH matches the held ETH after recovering. Therefore an admin (or the timelock) can (intentionally or unintentionally) withdraw ETH without burning frxETH.

The following test successfully runs when added to frxETHMinterTest. FRAX_COMPTROLLER successfully withdraws 5 Ethers deposited, while the user still has their original 5 frxETH minted.

function testBadRecover() public {
    address user = 0x1234567890123456789012345678901234567890;
    
    vm.startPrank(user);
    vm.deal(user, 5 ether);
    minter.submit{value: 5 ether}();
    vm.stopPrank();

    vm.startPrank(FRAX_COMPTROLLER);

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

    // Recover 5 ETH 
    vm.expectEmit(false, false, false, true);
    emit EmergencyEtherRecovered(5 ether);
    minter.recoverEther(5 ether);

    // FRAX_COMPTROLLER gets 5 ether, but the user's frxETH balance is the same
    assertEq(FRAX_COMPTROLLER.balance, starting_eth + (5 ether));
    assertEq(frxETHToken.balanceOf(user), 5 ether);

    vm.stopPrank();
}

Impact

A malicious or compromised admin can withdraw ETH directly without burning frxETH, inflating frxETH and de-pegging the token by its definition.

Tools Used

VSCode

Recommended Mitigation Steps

If there were only one minter, this check would have been enough:

require(amount <= address(this).balance - frxETHToken.totalSupply() - currentWithheldETH, "Not enough for recover");

Basically we want to account for frxETH's total supply, as well as the current ETH withheld by the contract. Having multiple minters makes checking substantially more difficult. A few possible ideas are:

• Creating a frxETH by minters management contract for such case.
• Deposit ETH directly into frxETH, similar to wrapped ETH. Thus effectively enforcing frxETH total supply == ETH deposited. For the withheld ETH for onboarding validators, one can also use frxETH as a central contract for holding, and minters would be able to request 32 ETHs at a time from frxETH for onboarding new validators.
• Enforce having only one minter (is there a scenario where we need multiple minters?).

The third idea would be the simplest in my opinion, then such a check as shown above would be enough.

[L-01] There is no way to submit ETH for without minting frxETH

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L114-L116 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L104-L106

frxETHMinter.receive() is equivalent to submit(), thereby there is no way for (e.g.) governance to submit ETH for withholding, except for forcefully sending ETH by self-destructing a contract.

[L-02] It is still possible for an uncontrolled address to be owner

The Owned contract is designed such that any new owner must accept ownership, most likely to prevent mistaken transfers. However, this is not the case for the constructor, as owner is set to a parameter address, as opposed to the deployer (msg.sender).

https://github.com/code-423n4/2022-09-frax/blob/main/src/Utils/Owned.sol#L10-L14

The Owned.sol file is out-of-scope, however both OperatorRegistry and frxETHMinter contracts uses the same method for setting the initial owner. Thus the deployed contract can be lost upon construction if the initial address is not appropriately set.

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L40-L43 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L52-L65

For this reason I consider this a low-severity finding, and am opting to bring Owned.sol into the scope for this specific case.

Recommended mitigation steps: Use msg.sender as the initial owner, as opposed to a parameter.

Also consider using boringcrypto's BoringOwnable.sol, which enforces the initial owner to be the deployer. The contract has all functionalities needed, and it's also good practice to use prewritten tools that are known to be secure.

[L-03] ERC20PermitPermissionedMint.sol: It is possible to implement removeMinter() without having to loop through the entire array

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L76-L92

Loop line: https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L84

In general, it is not good practice to loop through an entire storage array, as it would cause the tx to be gas-costly, or even reverting, although such a case would be quite extreme and not likely to happen.

This is considered a low finding because it has a gas optimization, a QA element, as well as a possible (but unlikely) bug.

Recommended mitigation steps: We can use the minters mapping to store the ($1$-based) index of the minter, instead of simply a boolean flag. This will immediately tell us which position to set minters_array to $0$, and also has the virtue of saving gas, for not having to read the entire storage array, as well as using uint256 is actually more gas-efficient than bool.

Such an implementation can be as follow:

address[] public minters_array;
mapping(address => uint256) public minters; // not bool

function addMinter(address minter_address) public onlyByOwnGov {
    minters[minter_address] = minters_array.length + 1; 
    minters_array.push(minter_address);
}

function removeMinter(address minter_address) public onlyByOwnGov {
    require(minters[minter_address] != 0, "Address nonexistant");
    minters_array[minters[minter_address]] = address(0);
    delete minters[minter_address];
}

The onlyMinters modifier would then also need to be changed, albeit that too is quite simple

modifier onlyMinters() {
    require(minters[msg.sender] != 0, "Only minters");
    _;
} 

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L45-L48

Alternatively, one can also take an approach to remove minter by index, instead of by address (similar to how OperatorRegistry.removeValidator() works), so that the minters mapping can retain a boolean-like value structure (however that would require finding the minter's index off-chain first).

[L-04] It is recommended to use safeTransfer instead of transfer for ERC20

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

[N-01] Inconsistent spacing between brackets

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L84-L85

[N-02] Function names can be more informative

OperatorRegistry.getNextValidator() can be renamed to getLastValidatorAndPop()

• The current function name does not mention popping behavior.
• "Next" is usually not interpreted to be "Last".

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L126

frxETHMinter.depositEther() can be renamed to deliverEtherToValidators(), or depositWithheldETHToValidators():

• "Deposit Ether" doesn't make a very descriptive name.
• The word "deposit" in a DeFi context is often used when the tx sender deposits funds into a contract, thus the current usage is rather unconventional.

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

[N-03] public functions that are not called by the contract itself can be made external

Applies to all public functions within ERC20PermitPermissionedMint.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L53 https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L59 https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L65 https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L76 https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L94

[G-01] Using bool instead of uint256 causes extra gas overhead

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L20 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L43

[G-02] Don't compare array.length in every iteration of the for loop

It is more gas-efficient to cache the length first, then compare them. E.g.

uint len = minters_array.length;
for (uint i = 0; i < len; i++) { 
    if (minters_array[i] == minter_address) {
        minters_array[i] = address(0);
        break;
    }
}

Two applicable instances:

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L83-L89 https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L114

[G-03] Don't compare a boolean with true, use the value itself

e.g. This line of code

require(minters[msg.sender] == true, "Only minters");

Can be changed to

require(minters[msg.sender], "Only minters");

Or, if [G-01] is implemented

require(minters[msg.sender] != 0, "Only minters");

Two applicable instances

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L46 https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L78

[G-04] Comparing != 0 instead of > 0 for uint type is more gas-efficient

Two applicable instances

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