Spell accepts multiple instances of the same strategy
Summary
BasicSpell's addStrategy() does not validate if the vault was already listed as part of strategies under this spell.
As such, it is possible to violate the validation of MaxPositionSize since, strategyId could be different with different max position, while the balance of token for the spell contract will be sum of multiple instances of strategies.
EOA1 use strategyId1 to add a position for 55;
spellBalance = strategyA.vault.balanceOf(address(this));
console.log(spellBalance) = 55;
EOA2 use strategyId2 to add a position for 95;
spellBalance = strategyA.vault.balanceOf(address(this));
console.log(spellBalance) = 55 + 95 = 150;
Impact
using the two different strategy ids, the spell balance went above maxPositionSize causing issues in almost every spell in openPositionFarm() function. New Open positions in the spell may start to fail.
existingStrategy modifier should be based on vault address
or in the _addStrategy call, check if the vault address is already existing in strategies array before adding it to the strategy list. Add a new strategy only if the vault is not found in all the strategies added to the current spell contract.
ravikiran.web3
medium
Spell accepts multiple instances of the same strategy
Summary
BasicSpell's addStrategy() does not validate if the vault was already listed as part of strategies under this spell.
As such, it is possible to violate the validation of MaxPositionSize since, strategyId could be different with different max position, while the balance of token for the spell contract will be sum of multiple instances of strategies.
Vulnerability Detail
Example: Lets say strategyA = { vault:"0x0aaab...", maxPositionSize: 100} strategyId1 = AuraSpell.addStrategy(strategyA.vault, strategyA.maxPositionSize);
now, strategyA2 = { vault:"0x0aaab...", maxPositionSize: 125 } strategyId2 = AuraSpell.addStrategy(strategyA2.vault, strategyA2.maxPositionSize);
EOA1 use strategyId1 to add a position for 55; spellBalance = strategyA.vault.balanceOf(address(this)); console.log(spellBalance) = 55;
EOA2 use strategyId2 to add a position for 95; spellBalance = strategyA.vault.balanceOf(address(this)); console.log(spellBalance) = 55 + 95 = 150;
Impact
using the two different strategy ids, the spell balance went above maxPositionSize causing issues in almost every spell in openPositionFarm() function. New Open positions in the spell may start to fail.
Code Snippet
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/BasicSpell.sol#L36-L39
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/BasicSpell.sol#L198-L207
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/AuraSpell.sol#L127
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ConvexSpell.sol#L125
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/CurveSpell.sol#L121
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/IchiSpell.sol#L108
https://github.com/sherlock-audit/2023-04-blueberry/blob/main/blueberry-core/contracts/spell/ShortLongSpell.sol#L105
Tool used
Manual Review
Recommendation
existingStrategy modifier should be based on vault address
or in the _addStrategy call, check if the vault address is already existing in strategies array before adding it to the strategy list. Add a new strategy only if the vault is not found in all the strategies added to the current spell contract.