code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/d86fe0a9959c2b43c62716240d981ae95224e49e/src/CVXStaker.sol#L118

Vulnerability details

Impact

Missing check booster.isShutdown==false, may cause deposit to fail

Proof of Concept

isCvxShutdown() is used to determine if CVX is shutdown

The code is as follows:

    /**
     * @dev Checks whether the CVX pool is currently shutdown.
     * @return A boolean indicating whether the CVX pool is currently shutdown.
     */
    function isCvxShutdown() public view returns (bool) {
        // It's not necessary to check that the booster itself is shutdown, as that can only
        // be shutdown once all the pools are shutdown - see Cvx BoosterOwner.shutdownSystem()
        return booster.poolInfo(cvxPoolInfo.pId).shutdown;
    }

The current implementation only determines whether the pool is shutdown, and does not check whether the booster is shutdown The code comments explain that the execution of BoosterOwner.shutdownSystem() will ensure that the all pools has been shutdown

        // It's not necessary to check that the booster itself is shutdown, as that can only
        // be shutdown once all the pools are shutdown - see Cvx BoosterOwner.shutdownSystem()

But in BoosterOwner, it is possible to force shutdown to the booster use BoosterOwner.forceShutdownSystem(), in some special cases

The following code is from BoosterOwner.sol

url: https://etherscan.io/address/0x3cE6408F923326f81A7D7929952947748180f1E6#code

    //queue a forced shutdown that does not require pools to already be shutdown
    //this should only be needed if a pool is broken and withdrawAll() does not
    //correctly return enough lp tokens
    function queueForceShutdown() external onlyOwner{
        require(IOwner(poolManager).isShutdown(),"!poolMgrShutdown");
        require(!isForceTimerStarted, "already started");

        isForceTimerStarted = true;
        forceTimestamp = block.timestamp + FORCE_DELAY;

        emit ShutdownStarted(forceTimestamp);
    }

    //force shutdown the system after timer has expired
    function forceShutdownSystem() external onlyOwner{
        require(isForceTimerStarted, "!timer start");
        require(block.timestamp > forceTimestamp, "!timer finish");

        IOwner(booster).shutdownSystem();
        emit ShutdownExecuted();
    }

booster.shutdownSystem() code:

https://etherscan.io/address/0xF403C135812408BFbE8713b5A23a04b3D48AAE31#code

    //shutdown this contract.
    //  unstake and pull all lp tokens to this address
    //  only allow withdrawals
    function shutdownSystem() external{
        require(msg.sender == owner, "!auth");
        isShutdown = true;

        for(uint i=0; i < poolInfo.length; i++){
            PoolInfo storage pool = poolInfo[i];
            if (pool.shutdown) continue;

            address token = pool.lptoken;
            address gauge = pool.gauge;

            //withdraw from gauge
            try IStaker(staker).withdrawAll(token,gauge){
                pool.shutdown = true;
            }catch{}//<-------@audit revert will shutdown == false
        }
    }

From the above code we know that if withdrawAll() fails, then pool.shutdown==false , but booster.isShutdown==true

To sum up, if BoosterOwner executes forceShutdownSystem() for some reason then there will still be the case: booster.shutdown == true , but pool.shutdown == false

If this happens, it will cause isCvxShutdown() == false, but in CVXStaker.depositAndStake() it will revert, because booster.isShutdown==true

which causes AMO2.sol not to work properly

Tools Used

Recommended Mitigation Steps

    function isCvxShutdown() public view returns (bool) {
...
-       return booster.poolInfo(cvxPoolInfo.pId).shutdown;
+       return (booster.isShutdown || poolInfo(cvxPoolInfo.pId).shutdown);
    }

Assessed type

Context

c4-sponsor commented 1 year ago

vaporkane marked the issue as disagree with severity

c4-sponsor commented 1 year ago

vaporkane marked the issue as sponsor confirmed

kirk-baird commented 1 year ago

This is a valid issue however it does not have any meaningful impact on users of the protocol.

For the edge case that is missing, a booster is shutdown but not a specific pool the call to booster.deposit() will revert. The revert prevents funds incorrectly being transferred to the shutdown pool and prevents any meaningful attacks.

Therefore since there is minimal impact for this bug I consider it low severity.

c4-judge commented 1 year ago

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