The protectAddPool is unsafe

PoolManagerLite.sol has ported over a feature from the initial pool manager that allows the operator role to remove authentication for the function addPool.

function addPool(address _gauge, uint256 _stashVersion) external returns (bool) {
    return _addPool(_gauge, _stashVersion);
}

function _addPool(address _gauge, uint256 _stashVersion) internal returns (bool) {
    require(!IPools(booster).gaugeMap(_gauge), "already registered gauge");
    require(!isShutdown, "shutdown");

    if (protectAddPool) {
        require(msg.sender == operator, "!auth");
    }

    address lptoken = ICurveGauge(_gauge).lp_token();
    require(!IPools(booster).gaugeMap(lptoken), "already registered lptoken");

    return IPools(booster).addPool(lptoken, _gauge, _stashVersion);
}

By default this is on, but we believe this should never be disabled. Giving everyone the possibility to add pools leads to a few dangerous scenarios.

The IPools(booster).addPool(lptoken, _gauge, _stashVersion) ends up doing poolInfo.push of a PoolInfo object. There are no checks on the _gauge param except to check if it is already added and nonzero. The PoolInfo object can only ever be pushed to and individual pools can be shut down, but items are never popped off. In functions like shutdownSystem, this array is iterated over.

function shutdownSystem() external {
    require(msg.sender == owner, "!auth");
    isShutdown = true;

    for (uint256 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 {}
    }
}

If poolInfo is too large, the loop will run out of gas before it can finish. This blocks the possibility to run shutdownSystem() if someone has spammed the pool manager with random pools that implement the required view functions that are checked.

The same happens in BoosterOwnerLite.sol in its shutdownSystem() function, where it loops to IOwner(booster).poolLength(). The consequence is the same, and shutting down might be impossible or very costly in terms of gas.

When discussing the issue with Aura Finance, they mentioned that this functionality can be removed, as the protection is always enabled in the sidechain.