depositEther function DoS with locking funds

[code423n4]

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.

[FortisFortuna]

We plan to keep an eye on the number free validators and have a decent sized buffer of them.

[FortisFortuna]

duplicate of https://github.com/code-423n4/2022-09-frax-findings/issues/286

[FortisFortuna]

Adding a maxLoops parameter or similar can help mitigate this for sure.

[0xean]

dupe of #17 / #224