Booster's shutdownPool can freeze user funds

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L360-L365

Vulnerability details

shutdownPool marks shutdown successful even if it's not (i.e. withdrawAll() call reverted). As withdrawing logic expect that the pool in shutdown has already provided the funds, and make no additional attempts to retrieve them, user funds will be frozen until shutdown state be turned off for the pool.

Proof of Concept

In the case of unsuccessful withdrawAll the pool nevertheless will be marked as shutdown:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L360-L365

    //withdraw from gauge
    try IStaker(staker).withdrawAll(pool.lptoken,pool.gauge){
    }catch{}

    pool.shutdown = true;
    gaugeMap[pool.gauge] = false;

It will block the withdrawals as there will be not enough funds to fulfill the claim:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L462-L476

    //pull from gauge if not shutdown
    // if shutdown tokens will be in this contract
    if (!pool.shutdown) {
        IStaker(staker).withdraw(lptoken,gauge, _amount);
    }

    //some gauges claim rewards when withdrawing, stash them in a seperate contract until next claim
    //do not call if shutdown since stashes wont have access
    address stash = pool.stash;
    if(stash != address(0) && !isShutdown && !pool.shutdown){
        IStash(stash).stashRewards();
    }

    //return lp tokens
    IERC20(lptoken).safeTransfer(_to, _amount);

This way user funds will be frozen as the system will not attempt to withdraw from the pool, while there will be no funds to transfer to user and _withdraw() will be reverting on L476 transfer call.

Recommended Mitigation Steps

The shutdownPool logic can to be updated:

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L356-L369

Consider unifying the logic with shutdownSystem() and marking the pool shutdown only if withdraw was successful, for example:

function shutdownPool(uint256 _pid, bool forced) external returns(bool){
    require(msg.sender==poolManager, "!auth");
    PoolInfo storage pool = poolInfo[_pid];

    bool withdrawSuccess = false;

    //withdraw from gauge
    try IStaker(staker).withdrawAll(pool.lptoken,pool.gauge){
        withdrawSuccess = true;
    }catch{}

    if (withdrawSuccess || forced) {
        pool.shutdown = true;
        gaugeMap[pool.gauge] = false;
        emit PoolShutdown(_pid);
        return true;
    }

    return withdrawSuccess;
}

[0xMaharishi]

immutable PoolManagerSecondaryProxy already enforces this

[dmvt]

Enforced upstream by the permissioned caller. Invalid.