hunter_w3b - `StrategyPassiveManagerVelodrome::setRewardPool` Does Not Remove ERC20 Token Allowances When rewardPool is Updated

[sherlock-admin3]

hunter_w3b

medium

StrategyPassiveManagerVelodrome::setRewardPool Does Not Remove ERC20 Token Allowances When rewardPool is Updated

Summary

StrategyPassiveManagerVelodrome provides ERC20 token allowances to the rewardPool, but these allowances are not removed when the rewardPool is updated.

Vulnerability Detail

When initialize is called, the rewardPool is assigned an address, and subsequently, the initialize function gives a maximum allowance to the rewardPool.

    function initialize (
        address _pool,
        address _quoter,
        address _nftManager,
        address _gauge,
        address _rewardPool,
        address _output,
        int24 _positionWidth,
        bytes[] calldata _paths,
        CommonAddresses calldata _commonAddresses
    ) external initializer {
        __StratFeeManager_init(_commonAddresses);

        //...

@>>        rewardPool = _rewardPool;

        //...

@>>        _giveAllowances();

    }

When the owner of the contract wants to update the rewardPool, the rewardPool has already been approved with the maximum value. This allows the contract to enter a state where the rewardPool is updated via setRewardPool, but the ERC20 token approvals given to the old rewardPool are not removed.

    function setRewardPool(address _rewardPool) external onlyOwner {
        rewardPool = _rewardPool;
        emit SetRewardPool(_rewardPool);
    }

    function _giveAllowances() private {
        IERC20Metadata(output).forceApprove(unirouter, type(uint256).max);
@>>        IERC20Metadata(output).forceApprove(rewardPool, type(uint256).max);
        IERC20Metadata(lpToken0).forceApprove(nftManager, type(uint256).max);
        IERC20Metadata(lpToken1).forceApprove(nftManager, type(uint256).max);
    }

Allowances can only be removed by calling StrategyPassiveManagerVelodrome::panic. However, rewardPool can be changed at any time via the setRewardPool function.

    function panic(uint256 _minAmount0, uint256 _minAmount1) public onlyManager {
        _claimEarnings();
        _removeLiquidity();
@>>        _removeAllowances();
        _pause();

        (uint256 bal0, uint256 bal1) = balances();
        if (bal0 < _minAmount0 || bal1 < _minAmount1) revert TooMuchSlippage();
    }

Therefore, to remove the allowance, the entire protocol must be paused to remove the allowance from rewardPool.

This allows the contract to enter a state where rewardPool is updated via setRewardPool but the ERC20 token approvals given to the old rewardPool are not removed.

Impact

The old rewardPool contract will continue to have ERC20 token approvals, allowing it to continue spending the protocol’s tokens when this is not intended, as the protocol has changed the rewardPool.

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/main/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L776-L779

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/main/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L827C1-L832C6

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/main/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L173-L188

Tool used

Manual Review

Recommendation

This issue is correctly handled in the setUnirouter function.

    function setUnirouter(address _unirouter) external override onlyOwner {
        _removeAllowances();
        unirouter = _unirouter;
        _giveAllowances();
        emit SetUnirouter(_unirouter);
    }

A similar approach should be implemented in the setRewardPool function.

    function setRewardPool(address _rewardPool) external onlyOwner {
+        _removeAllowances();
        rewardPool = _rewardPool;
+        _giveAllowances();
        emit SetRewardPool(_rewardPool);
    }

Duplicate of #3