Frax Ether Liquid Staking contest - V_B's results

Lines of code

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

Vulnerability details

Description

There is a removeMinter function in ERC20PermitPermissionedMint. The function performs the removal minter_address from the special address list. In other words, the function is needed to remove special access for a specific address.

// Remove a minter 
function removeMinter(address minter_address) public onlyByOwnGov {
    require(minter_address != address(0), "Zero address detected");
    require(minters[minter_address] == true, "Address nonexistant");

    // Delete from the mapping
    delete minters[minter_address];

    // 'Delete' from the array by setting the address to 0x0
    for (uint i = 0; i < minters_array.length; i++){ 
        if (minters_array[i] == minter_address) {
            minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
            break;
        }
    }

    emit MinterRemoved(minter_address);
}

Minter addresses are stored in two structures:

• mapping(address => bool) minters
• address[] minters_array

So the function should remove the address from both of them.

The removal from the first structure was implemented as delete minters[minter_address]. Please note, it is always consumed at most gas as one sstore can consume, so that is safe in terms of DoS attack.

The removing from the second structure is implemented as traversing the minters_array until the array element will be equal to the removing address and then assigning this element to address(0). Please note, traversing storage array consume as much gas as sload of all elements will cost, it can lead to DoS attack.

for (uint i = 0; i < minters_array.length; i++){ 
    if (minters_array[i] == minter_address) {
        minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
        break;
    }
}

If minters_array will contain many elements then traversing all of them may require more gas than is available in one Ethereum block! It can lead to the inability to remove the minter address.

Also, note that the size of the array is never reduced, the elements are simply set to zero, which makes the DoS more probable. Even if the insolvency will not happen, the gas consumption is still much higher than it can be, which will complicate the removal of minters and replacing them with others.

PoC

// SPDX-License-Identifier: GPL-2.0-or-later
pragma solidity ^0.8.0;

contract ERC20PermitPermissionedMint {
    address[] public minters_array; // Allowed to mint
    mapping(address => bool) public minters; // Mapping is also used for faster verification

    event MinterAdded(address minter_address);
    event MinterRemoved(address minter_address);

    function addMinter(address minter_address) public {
        require(minter_address != address(0), "Zero address detected");

        require(minters[minter_address] == false, "Address already exists");
        minters[minter_address] = true; 
        minters_array.push(minter_address);

        emit MinterAdded(minter_address);
    }

    function removeMinter(address minter_address) public {
        require(minter_address != address(0), "Zero address detected");
        require(minters[minter_address] == true, "Address nonexistant");
        
        // Delete from the mapping
        delete minters[minter_address];

        // 'Delete' from the array by setting the address to 0x0
        for (uint i = 0; i < minters_array.length; i++){ 
            if (minters_array[i] == minter_address) {
                minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
                break;
            }
        }

        emit MinterRemoved(minter_address);
    }
}

contract PoC is ERC20PermitPermissionedMint {
    uint256 addedMinters;
    
    function addManyAddresses(uint256 x) external  {
        uint256 currentMinter = addedMinters;
        for(uint256 i = currentMinter + 1; i <= currentMinter + x; ++i) {
            addMinter(address(uint160(i)));
        }

        addedMinters = currentMinter + x;
    }
}

The contract above is inherited from ERC20PermitPermissionedMint, with a modification to add many minters at a time. With this modification, it is easy to check how much gas will be consumed for the removeMinter function.

Here is some test that we have made on Solidity version 0.8.17 with enabled 200 optimizations:

Recommended Mitigation Steps

Remove element from minters_array by swapping such element with the last one and calling pop on the array.

Lines of code

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

Vulnerability details

Description

There is a depositEther function in frxETHMinter contract. The function performs multiple deposits to the depositContract. More detailed, the contract calculates the amount of ether that was submitted to it, and everything, except withheld amount, is deposited to depositContract. Deposits are made as separate calls to the depositContract each with 32 ether value.

/// @notice Deposit batches of ETH to the ETH 2.0 deposit contract
/// @dev Usually a bot will call this periodically
function depositEther() external nonReentrant {
    // Initial pause check
    require(!depositEtherPaused, "Depositing ETH is paused");

    // See how many deposits can be made. Truncation desired.
    uint256 numDeposits = (address(this).balance - currentWithheldETH) / DEPOSIT_SIZE;
    require(numDeposits > 0, "Not enough ETH in contract");

    // Give each deposit chunk to an empty validator
    for (uint256 i = 0; i < numDeposits; ++i) {
        // Get validator information
        (
            bytes memory pubKey,
            bytes memory withdrawalCredential,
            bytes memory signature,
            bytes32 depositDataRoot
        ) = getNextValidator(); // Will revert if there are not enough free validators

        // Make sure the validator hasn't been deposited into already, to prevent stranding an extra 32 eth
        // until withdrawals are allowed
        require(!activeValidators[pubKey], "Validator already has 32 ETH");

        // Deposit the ether in the ETH 2.0 deposit contract
        depositContract.deposit{value: DEPOSIT_SIZE}(
            pubKey,
            withdrawalCredential,
            signature,
            depositDataRoot
        );

        // Set the validator as used so it won't get an extra 32 ETH
        activeValidators[pubKey] = true;

        emit DepositSent(pubKey, withdrawalCredential);
    }
}

According to the code, the number of deposits performed by the function is calculated by the following formula:

// See how many deposits can be made. Truncation desired.
uint256 numDeposits = (address(this).balance - currentWithheldETH) / DEPOSIT_SIZE;

The deposits are performed in a loop with numDeposits iterations, without any upper bound on the number of iterations from the caller side. Please note, that non-malicious users actively increase the number of iterations in this loop just by using the deposit logic of the contract. In other words, the more users submit funds to the contract the heavier (in terms of gas amount) call of the depositEther will be.

Let's consider the case when the activity of contract users is high and they deposit many ether value between calls of depositEther function. In such a scenario, the function may consume more gas than the Ethereum block has. This means that in this case the function can never be called successfully.

Please note, that there is an access-controlled function recoverEther that allows the owner to transfer ether from the contract, and therefore make the logic alive. Although such a possibility exists, it is still considered as unintentional DoS and should be fixed toward safer design.

Also it should be taken into account, that it is possible to send to the contract some value using selfdestruct(payable(contract_address)) instruction to increase (address(this).balance - currentWithheldETH) difference in the most significant way.

Mitigation steps

Add an input parameter to be used as the upper bound on the number of deposits to be processed inside of depositEther function.

1. usage of non-ERC20 standard function "permit"

It is unsafe to rely on logic that underlying token have permit function implemented. It is considered good practice to use ERC20 standard modification that contains logic of safe calling of permit function. This is so, because of possibility of existence of phantom function that will be used when contract calls permit and, as result, incorrect logic of usage of such function.

2. indexed parameters of events

It is reasonable to use indexed for some of the parameters of the events EmergencyERC20Recovered, TimelockChanged, ValidatorsSwapped, MinterAdded, MinterRemoved, TimelockChanged, OwnerNominated, OwnerChanged.

3. possibility of arbitrary call for owner account

It is reasonable to add allowance for the owner account to call any contract by call/delegate call on behalf of contract. As we can see owner already have an ability to withdraw any amount of ETH or any ERC20 token, so it will be good to give owner rights to call any logic and to not have many special recover functions.

4. external minter_burn_from and minter_mint functions

minter_burn_from and minter_mint functions declared as public, but it is reasonable to delcare them as external, since they are not used throught internal calls.

5. clearValidatorArray function DoS

There is a function clearValidatorArray in OperatorRegistry contract. It clears all the validator array, so for each of the validators it writes to the corresponding storage slot a zero value. In case of big amount of validators there will be no chance to call this function due to very big amount of gas to be used, which actually can be even greater than the block gas limit.

1. calldata for parameters instead of memory in external functions

It is reasonable to use calldata instead of memory for input arrays in setWithdrawalCredential and getValidatorStruct functions to reduce gas consumption and make the code more clear.

2. removeValidator function gas optimization

It will be more efficient to use a little bit different logic for removeValidator function from OperatorRegistry.sol contract.

There is the following logic for the case when dont_care_about_ordering input parameter is false:

// Save the original validators
Validator[] memory original_validators = validators;

// Clear the original validators list
delete validators;

// Fill the new validators array with all except the value to remove
for (uint256 i = 0; i < original_validators.length; ++i) {
    if (i != remove_idx) {
        validators.push(original_validators[i]);
    }
}

It is reasonable to not clear all the array and do redundant reads and writes from the storage, but instead this implement the following logic:

for (uint256 i = remove_idx; i + 1 < validators.length; ++i) {
    validators[i] = validators[i + 1];
}
validators.pop();