Frax Ether Liquid Staking contest - Chom's results

Lines of code

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

Vulnerability details

Impact

Governor can rug all unstaked ether in contract using recoverEther. This causes fund loss and state failures. A case of failure is frxETH will become unbacked by real staked ETH since it is rugged by an admin.

Proof of Concept

    /// @notice For emergencies if something gets stuck
    function recoverEther(uint256 amount) external onlyByOwnGov {
        (bool success,) = address(owner).call{ value: amount }("");
        require(success, "Invalid transfer");

        emit EmergencyEtherRecovered(amount);
    }

recoverEther is not checking for active ETH supply. Admin can withdraw all ETH in the contract.

Recommended Mitigation Steps

recoverEther should be able to recover only fund that over the remaining supply = total supply of frxETH - deposited amount - currentWithheldETH

Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L78-L97

Vulnerability details

Impact

syncRewards must be called on all cycles. If one cycle is missed, the reward may be miscalculated.

Proof of Concept

    function syncRewards() public virtual {
        uint192 lastRewardAmount_ = lastRewardAmount;
        uint32 timestamp = block.timestamp.safeCastTo32();

        if (timestamp < rewardsCycleEnd) revert SyncError();

        uint256 storedTotalAssets_ = storedTotalAssets;
        uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;

        storedTotalAssets = storedTotalAssets_ + lastRewardAmount_; // SSTORE

        uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

        // Combined single SSTORE
        lastRewardAmount = nextRewards.safeCastTo192();
        lastSync = timestamp;
        rewardsCycleEnd = end;

        emit NewRewardsCycle(end, nextRewards);
    }

lastRewardAmount is the amount of rewards distributed in the most recent cycle. If a cycle is skipped, the lastRewardAmount of skipped cycle won't be calculated. It May cause reward lost if asset balance is high and cause reward to be higher than expected if asset balance is low.

Recommended Mitigation Steps

Ensure that this function will always run on every cycle.

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L114-L118

Vulnerability details

Impact

removeValidator without dont_care_about_ordering may always revert if there are too many validators. Due to gas limit.

Proof of Concept

            // 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]);
                }
            }

These lines in the OperatorRegistry contract will be reverted with the gas limit if the original_validators count is too high

Recommended Mitigation Steps

Limit the number of validators

Lines of code

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

Vulnerability details

Impact

recoverERC20 is using an unsafe transfer to transfer the token. Combined with require, this cause USDT to be completely non-recoverable. This is a fund loss.

Proof of Concept

USDT transfer function doesn't return anything as seen in https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code

    // Forward ERC20 methods to upgraded contract if this one is deprecated
    function transfer(address _to, uint _value) public whenNotPaused {
        require(!isBlackListed[msg.sender]);
        if (deprecated) {
            return UpgradedStandardToken(upgradedAddress).transferByLegacy(msg.sender, _to, _value);
        } else {
            return super.transfer(_to, _value);
        }
    }

If tokenAddress is USDT, IERC20(tokenAddress).transfer(owner, tokenAmount) will return nothing and will always revert with "recoverERC20: Transfer failed" due to not returning anything.

require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");

Recommended Mitigation Steps

Use safeTransfer from the SafeERC20 library. Remind to remove required statement too since safeTransfer may not return boolean success but it has already checked for transfer success.

IERC20(tokenAddress).safeTransfer(owner, tokenAmount);

Lines of code

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

Vulnerability details

Impact

recoverEther will cause currentWithheldETH to be wrong. Further deposits will lose a lot of ETH!

For example, if the admin withdraws all ETH using recoverEther. Assume we have 1024 ETH in the currentWithheldETH. After that, somebody deposits 64 ETH. This 64 ETH will cover the currentWithheldETH bad debt instead of can be used to stake to the validators. You need to waste 1024 ETH to cover the currentWithheldETH bad debt.

Proof of Concept

Assume we have 1024 ETH in the currentWithheldETH

    function recoverEther(uint256 amount) external onlyByOwnGov {
        (bool success,) = address(owner).call{ value: amount }("");
        require(success, "Invalid transfer");

        emit EmergencyEtherRecovered(amount);
    }

Admin can recover all ether as it doesn't check against currentWithheldETH. Assume admin withdraws all ethers.

currentWithheldETH is still the same since it hasn't been set.

Assume someone deposits 64 ETH after admin withdrawal.

    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);
        }
    }

address(this).balance - currentWithheldETH = 64 - 1024 will be minus. We need to deposit 1024 ETH back to make it not a minus.

Recommended Mitigation Steps

Emergency should be limited to only after toggling an emergency switch. The emergency switch should pause the entire contract.

1. Create an emergency switch that pauses the entire contract (Migrate the old one to a new one)
2. Check that emergency switch before allowing recoverEther.
3. Add a function to withdraw staked funds from validators if the emergency switch is on.
4. Remember to migrate frxETHToken and sfrxETHToken too.

Using assembly to optimize ETH sending

Change

        (bool success,) = payable(to).call{ value: amount }("");
        require(success, "Invalid transfer");

to

        bool success;

        assembly {
            // Transfer the ETH and store if it succeeded or not.
            success := call(gas(), to, amount, 0, 0, 0, 0)
        }

        if(!success) revert ETH_TRANSFER_FAILED();

Use unchecked { ++i; }

for (uint256 i = 0; i < numDeposits; ++i) {

Should be

for (uint256 i = 0; i < numDeposits; ) {
    ...
    unchecked { ++i; }
}

Use custom errors instead of requires

require(!depositEtherPaused, "Depositing ETH is paused");

Should be

error Paused();
...
if (depositEtherPaused) revert Paused();