Duplicate or incorrect validators temporarily disable `depositEther`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L53 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L140 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L143

Vulnerability details

Impact

If the admin adds an incorrect or a duplicate validator, then depositEther will revert. It will stay disabled until the owner comes after a few hours to remove the validator causing problems. In the worst case, since frxETHMinter holds funds, the team could choose to set the owner to the zero address. In this case the frax timelock contract has a total delay of execution of 16 days, at the end of which deposits could be resumed.

Proof of Concept

For duplicate validators:

1. owner or timelock adds some validators by calling OperatorRegistry.addValidators() and doesn't pay attention that in the array passed as an argument there is a duplicate.
2. users stake until at some time the first of the duplicate validators added is used in depositEther(). It is registered to being active after deposit (activeValidators[pubKey] = true;)
3. users continue to stake and at a certain moment when depositEther() is called the other duplicate validator is used, but this time the deposit will fail at the check require(!activeValidators[pubKey], "Validator already has 32 ETH");
4. last validator is removed by caling OperatorRegistry.removeValidator() by:
   • owner a few hours later
   • timelock 16 days later if owner was previously set to zero address

For an incorrect validator the description is similiar but depositEther fails at the call to depositContract.deposit.

Tools Used

Manual review

Recommended Mitigation Steps

Check for duplicate validators by declaring in OperatorRegistry:
mapping(bytes => bool) public isValidator
Then add the check in addValidator:

    function addValidator(Validator calldata validator) public onlyByOwnGov {
+       require(isValidator(validator.pubKey));
+       isValidator[pubKey] = true;
        validators.push(validator);
        emit ValidatorAdded(validator.pubKey, curr_withdrawal_pubkey);
    }

also add isValidator[pubKey] = false; each time a validator is popped.
For incorrect validators, add a try/catch block around the call to depositContract.deposit + remove the validator if the call fails:

    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
-               )
+           try 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);
+           } catch {
+               validators.pop()
+           }

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

-           emit DepositSent(pubKey, withdrawalCredential);
        }
    }```

[FortisFortuna]

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

[joestakey]

This could be a duplicate of #338 , however the warden describes a different issue:

If the admin adds an incorrect or a duplicate validator, then depositEther will revert. It will stay disabled until the owner comes after a few hours to remove the validator causing problems.

There is no mention of a malicious owner wanting to DoS on purpose depositEther- which is the only reason an admin would purposefully add a duplicate validator.

[0xean]

QA - see - https://github.com/code-423n4/2022-09-frax-findings/issues/338#issuecomment-1275401129