Frax Ether Liquid Staking contest - 8olidity's results

Lines of code

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

Vulnerability details

Impact

MoveWithheldETH () does not restrict the to address and may result in fewer currentWithheldETH

Proof of Concept

In normal logic, send the amount ETH to the TO address and then currentWithheldETH -= amount;

    function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov {
        require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");
        currentWithheldETH -= amount;

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

        emit WithheldETHMoved(to, amount);
    }

But if the to address is its own contract address address(this), it will fire

    receive() external payable {
        _submit(msg.sender);
    }

Run the _submit function

    function _submit(address recipient) internal nonReentrant {
        // Initial pause and value checks
        require(!submitPaused, "Submit is paused");
        require(msg.value != 0, "Cannot submit 0");

        // Give the sender frxETH
        frxETHToken.minter_mint(recipient, msg.value);

        // Track the amount of ETH that we are keeping
        uint256 withheld_amt = 0;
        if (withholdRatio != 0) {
            withheld_amt = (msg.value * withholdRatio) / RATIO_PRECISION;
            currentWithheldETH += withheld_amt;
        }

        emit ETHSubmitted(msg.sender, recipient, msg.value, withheld_amt);
    }

If withholdRatio is not set to the same size as RATIO_PRECISION, then withheld_amt will be smaller than the original amount

Tools Used

vscode

Recommended Mitigation Steps

add check

Lines of code

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

Vulnerability details

Impact

Token like USDT known for using non-standard ERC20. (Missing return boolean on transfer).

Contract function forwardERC20 will always revert when try to transfer this kind of tokens.

code4rena issue:https://github.com/code-423n4/2022-05-runes-findings/issues/70

Proof of Concept

    function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyByOwnGov {
        require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");

        emit EmergencyERC20Recovered(tokenAddress, tokenAmount);
    }

Tools Used

vscode

Recommended Mitigation Steps

Consider using OpenZepplin's SafeERC20 for the transfer calls.

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#L120

Vulnerability details

Impact

AddValidator () adds existing validators, which affect the calculation of numDeposits in depositEther()

Because every time getNextValidator() is evaluated, the validator is already active.

Proof of Concept

Add the validator repeatedly

    function addValidator(Validator calldata validator) public onlyByOwnGov {
        validators.push(validator);
        emit ValidatorAdded(validator.pubKey, curr_withdrawal_pubkey);
    }

The number of numDeposits are taken up. If the list is taken up repeatedly, it will take a lot of time to activate the Validator

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

Tools Used

vscode

Recommended Mitigation Steps

An active Validator cannot be added