Implement a short-circuit return flow to avoid a potential revert

Github username: @chainNue Twitter username: chainNue Submission hash (on-chain): 0x253b61a5ec1b171eb1bfd199fbdf4942aec3b7ab5f9158213f2bc58f20fd056e Severity: low

Description: Description

On _createLoot there is a _getQuestAllocationForPeriod which check if(nbQuestForGauge == 0 || questTotalRewards == 0) then return Allocation(0, 0), currently this zero allocation is not being used for short-circuit return flow.

For example when a user want to createLoot for future period, entering _getQuestAllocationForPeriod, it will make questTotalRewards = 0 (since it's not set yet), thus it will return Allocation(0, 0). If there is no short-circuit return if the allocation is 0, then on Line 484, it will revert due to divide by zero.

Instead of revert, which might need to be catched by front-end ui, it's normal to just return, just like everything else like:

if(!allowedDistributors[distributor]) return;
if(!ILootVoteController(lootVoteController).isListedGauge(vars.gauge)) return;
if(vars.userPeriodRewards == 0) return;

so, in this case, if(allocation == Allocation(0, 0)) return;

This early return is understandable, because the main op in _createLoot is the final line, creating loot, which main parameters are vars.userPalAmount and vars.userExtraAmount. These two parameters are affected by allocation.palPerVote & allocation.extraPerVote.

Further more, in Loot::createLoot(), there is if(palAmount == 0 && extraAmount == 0) return;, which is what we expect. Instead of consuming gas up to this point, it's better to immediate return when Allocation is 0.

File: LootCreator.sol
403:     function _getQuestAllocationForPeriod(
404:         address gauge,
405:         uint256 questId,
406:         address distributor,
407:         uint256 period
408:     ) internal view returns(Allocation memory) {
409:         address board = MultiMerkleDistributorV2(distributor).questBoard();
410:         uint256 nbQuestForGauge = IQuestBoard(board).getQuestIdsForPeriodForGauge(gauge, period).length;
411:         uint256 questTotalRewards = totalQuestPeriodRewards[distributor][questId][period];
412: 
413:  @>     if(nbQuestForGauge == 0 || questTotalRewards == 0) return Allocation(0, 0);
414: 
...
465:     function _createLoot(address user, address distributor, uint256 questId, uint256 period) internal {
...
472:         // Get Quest allocation
473:  @>     Allocation memory allocation = _getQuestAllocationForPeriod(vars.gauge, questId, distributor, period); 
...
482:         // Calculate ratios based on that
483:         vars.lockedRatio = (vars.userPower * UNIT) / vars.totalPower;
484:  @>     vars.rewardRatio = (vars.userPeriodRewards * UNIT) / totalQuestPeriodRewards[distributor][questId][period];

File: Loot.sol
238:     function createLoot(address user, uint256 startTs, uint256 palAmount, uint256 extraAmount) external nonReentrant onlyLootCreator {
239:  @>     if(palAmount == 0 && extraAmount == 0) return;

Attack Scenario

Attachments

Proof of Concept (PoC) File
Revised Code File (Optional)

File: LootCreator.sol
465:     function _createLoot(address user, address distributor, uint256 questId, uint256 period) internal {
...
472:         // Get Quest allocation
473:  @>     Allocation memory allocation = _getQuestAllocationForPeriod(vars.gauge, questId, distributor, period);
++           if(allocation.palPerVote == 0 && allocation.extraPerVote == 0) return;

Recommendation

Consider to add immediate return when Allocation is 0

hats-finance / Paladin-0x1610bfde27e57b068af7f38aec3d2a7b1d146989

Implement a short-circuit return flow to avoid a potential revert #60