BiasedMerc - StrategyPassiveManagerVelordrome::setRewardPool() doesn't set rewardPool allowance

[sherlock-admin3]

BiasedMerc

medium

StrategyPassiveManagerVelordrome::setRewardPool() doesn't set rewardPool allowance

Summary

When the owner calls setRewardPool with a new reward pool, the old approval for the old rewardPool is not cleared and the new rewardPool is not set an approval. Which means the new reward pool will not have an approval to spend the contract's tokens.

Vulnerability Detail

StrategyPassiveManagerVelodrome::setRewardPool()

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

StrategyPassiveManagerVelodrome.sol#L827-L840

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

    /// @notice removes swap permisions for the tokens from the unirouter.
    function _removeAllowances() private {
        IERC20Metadata(output).forceApprove(unirouter, 0);
>>      IERC20Metadata(output).forceApprove(rewardPool, 0);
        IERC20Metadata(lpToken0).forceApprove(nftManager, 0);
        IERC20Metadata(lpToken1).forceApprove(nftManager, 0);
    }

Impact

rewardPool will be unable to transfer output tokens from StrategyPassiveManagerVelordrom. The manager could technically use panic to clear allowances and then unpause however this would cause a lot of users to be worried about the state of the protocol and is clearly not what the function is intended for.

Code Snippet

StrategyPassiveManagerVelodrome::setRewardPool() StrategyPassiveManagerVelodrome.sol#L827-L840

Tool used

Manual Review

Recommendation

Utilise _removeAllowances and _giveAllowances just like in setUnirouter:

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

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

DHTNS commented:

Low -> depends on the fact that admin makes changes to rewardPool address in incorrect order consider the following order | ptrategyPassiveManagerVelodrome.panic() -> setRewardPool() -> unpause() | all the correct allowances are now in place with no funds lost that being said its a tough call

[MirthFutures]

This is a low. The order of operations would be panic, setRewardPool and unpause. It could improve flow by wrapping the setting of the rewardPool variable with _removeAllowances and _giveAllowances.

[MirthFutures]

https://github.com/beefyfinance/cowcentrated-contracts/pull/14

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/beefyfinance/cowcentrated-contracts/pull/14

[spacegliderrrr]

Fix looks good. Allowance is now set upon changing the rewardPool

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.