sherlock-audit / 2023-02-blueberry-judging

12 stars 5 forks source link

XKET - There is no way to remove/change existing strategies in IchiVaultSpell #274

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

XKET

medium

There is no way to remove/change existing strategies in IchiVaultSpell

Summary

IchiVaultSpell contract does not provide a way to remove/change existing strategies.

Vulnerability Detail

An IchiVaultSpell contract can have multiple strategies and each strategy is connected to a proper vault (IchiVault).

IchiVaultSpell.sol
72:     /**
73:      * @notice Owner privileged function to add vault
74:      * @param vault Address of ICHI angel vault
75:      * @param maxPosSize, USD price based maximum size of a position for given vault, based 1e18
76:      */
77:     function addStrategy(address vault, uint256 maxPosSize) external onlyOwner {
78:         if (vault == address(0)) revert ZERO_ADDRESS();
79:         if (maxPosSize == 0) revert ZERO_AMOUNT();
80:         strategies.push(Strategy({vault: vault, maxPositionSize: maxPosSize}));
81:         emit StrategyAdded(strategies.length - 1, vault, maxPosSize);
82:     }

I assume the protocol will deploy one IchiVaultSpell and attach multiple strategies to that Spell. The problem is that there is no way to disable/remove/change an existing strategy in the current implementation. I understand that the Spell can be blacklisted in the BlueBerryBank.sol but it will disbale all the strategies attached to the Spell. This can be a problem when there is a problem with a starategy. It is notable that there can be many IchiVault instances according to different Uniswap pool keys (i.e. token0, token1, fee), and it is possible that some vaults become useless or exploited in the future. In that case, the only option for the protocol is to blacklist the Spell and re-deploy with correct strategies attached again.

Impact

If a malicious/errorneous strategy is found, there is no way to blacklist that specific strategy. The protocol would need to stop the whole Spell deployment and re-deploy it with changed strategies.

Code Snippet

https://github.com/sherlock-audit/2023-02-blueberry/blob/main/contracts/spell/IchiVaultSpell.sol#L77

Tool used

Manual Review

Recommendation

Add a new variable whitelistedStrategies in the IchiVaultSpell contract to manage strategies.