befree3x - Old `rewardPool` address keeps having full permission to spend `output` token when new reward pool is set

[sherlock-admin2]

befree3x

medium

Old rewardPool address keeps having full permission to spend output token when new reward pool is set

Summary

StrategyPassiveManagerVelodrome gives output token allowances to rewardPool but doesn't remove allowance when new reward pool is configured.

Vulnerability Detail

StrategyPassiveManagerVelodrome gives full allowance to rewardPool inside the function _giveAllowances:

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);
}

A new reward pool can be set by the owner, as described below.

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

However, the given allowances are only removed by calling StrategyPassiveManagerVelodrome::panic or StrategyPassiveManagerVelodrome::setUnirouter functions. Without calling these two functions, the protocol owner could set a new reward pool address but the old one still has permission to spend the output token at will.

Impact

The old reward pool contract will continue to have ERC20 token approvals for StrategyPassiveManagerVelodrome so it can continue to spend the output tokens when this is not intended by the protocol.

Code Snippet

N/A

Tool used

Manual Review

Recommendation

It's recommended to remove all allowances on the output token of the old reward pool contract before changing to the new one.

function setRewardPool(address _rewardPool) external onlyOwner {
+   IERC20Metadata(output).forceApprove(rewardPool, 0);
    rewardPool = _rewardPool;
    emit SetRewardPool(_rewardPool);
}

Duplicate of #3