`harvestNextStrategy` can be optimized

[code423n4]

Handle

0xsanson

Vulnerability details

Impact

In Harvester.sol, harvestNextStrategy harvests the first strategy then shifts the array such that the second strategy becomes the first (so on future calls, all the strategy get harvested sequentially). A more efficient way (in terms of gas) to do this would be to have a state variable uint256 lastStrategyHarvest that starts at 0 and acts like an index for the last used strategy.

Tools Used

editor

Recommended Mitigation Steps

harvestNextStrategy would be rewritten like the following. Also removeStrategy may need some modifications depending on index.

function harvestNextStrategy(
    address _vault,
    uint256 _estimatedWETH,
    uint256 _estimatedYAXIS
)
    external
{
    require(canHarvest(_vault), "!canHarvest");
    uint256 k = strategies[_vault].addresses.length;    
    if (lastStrategyHarvest >= k) {
        lastStrategyHarvest = 0;
    }
    address strategy = strategies[_vault].addresses[lastStrategyHarvest];
    harvest(controller, strategy, _estimatedWETH, _estimatedYAXIS);
    lastStrategyHarvest = lastStrategyHarvest + 1;
    // solhint-disable-next-line not-rely-on-time
    strategies[_vault].lastCalled = block.timestamp;
}

[Haz077]

I like this solution, I think it's so better than shifting the array.

[GalloDaSballo]

Sponsor acknowledged