code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L43-L53 https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L78-L82 https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L170-L179

Vulnerability details

Impact

Lack of zero address check about the operator address in the setOperator and withdrawAllAndUnwrap funtions can cause the permanent loss of the lp tokens. Lack of zero address check about immutable var clpToken and booster in the constructor can cause the contract not to work properly and must be re-deployed to a new address.

Proof of Concept

setOperator set the operator address directly without any 0 address check.

    function setOperator(address _operator) external onlyOwner {
        operator = _operator;

        emit SetOperator(_operator);
    }

But the operator is used as the withdrawing target directly in the withdrawAllAndUnwrap. And it also has not any check about whether the operator is 0 address.


    function withdrawAllAndUnwrap(
        bool claim,
        bool sendToOperator
    ) external onlyOwner {
        IBaseRewardPool(cvxPoolInfo.rewards).withdrawAllAndUnwrap(claim);
        if (sendToOperator) {
            uint256 totalBalance = clpToken.balanceOf(address(this));
            clpToken.safeTransfer(operator, totalBalance);
        }
    }

If the owner calls the withdrawAllAndUnwrap function without setting a valid operator address before, all the clp token will be lost.

A similar lack of check occurs in constructor:

    IERC20 public immutable clpToken;
    ICVXBooster public immutable booster;
    constructor(
        address _operator,
        IERC20 _clpToken,
        ICVXBooster _booster,
        address[] memory _rewardTokens
    ) {
        operator = _operator;
        clpToken = _clpToken;
        booster = _booster;
        rewardTokens = _rewardTokens;
    }

The constructor sets the immutable vars clpToken and booster directly, which can cause the contract not to work properly and must be re-deployed to a new address.

Tools Used

Manual review

Recommended Mitigation Steps

Add zero address check.

Assessed type

Invalid Validation

kirk-baird commented 1 year ago

Zero address checks should be rated as QA issues. Although loss of funds may occur, it is an admin function which requires setting an operator to zero.

c4-judge commented 1 year ago

kirk-baird changed the severity to QA (Quality Assurance)

5z1punch commented 1 year ago

I agree, my negligence