Wrong order of requirements in `LootCreator::notifyDistributedQuestPeriod()` could allow users to double create loots under very specific circumstances

[hats-bug-reporter[bot]]

Github username: @JacoboLansac Twitter username: jacolansac Submission hash (on-chain): 0x3ed1a64b3cb2065711ffece86ef9a548430437f3046216ae7c946ae17d5b3723 Severity: medium

Description: There is no protection against two different distributors calling LootCreator::notifyDistributedQuestPeriod() for the same questId and the same period. When the first calls the function, the entire function will run, setting totalRewards in the totalQuestPeriodRewards mapping. When the second one calls the same function for the same questId and period, the input argument totalRewards is also set in totalQuestPeriodRewards[msg.sender][questId][period] = totalRewards before checking if the gauge has been already allocated for the period. This will lead to a weird state in which the totalQuestPeriodRewards mapping will have non-zero totalRewards for the two distributors.

    function notifyDistributedQuestPeriod(uint256 questId, uint256 period, uint256 totalRewards) external onlyAllowedDistributor nonReentrant {
        // Pull any new budget & update the current period to have an up to date budget
        _pullBudget();
        _updatePeriod();

        // Fetch the gauge for the quest & check if it's listed
        address gauge = _getQuestGauge(questId, msg.sender);
        if(!ILootVoteController(lootVoteController).isListedGauge(gauge)) return;

        // If not set yet, set the total rewards for the quest & period
@>      if(!totalQuestPeriodSet[msg.sender][questId][period]) {
@>          totalQuestPeriodRewards[msg.sender][questId][period] = totalRewards;
@>          totalQuestPeriodSet[msg.sender][questId][period] = true;
        }

        // If the period is already allocated for the gauge, return
@>      if(isGaugeAllocatedForPeriod[gauge][period]) return;
@>      isGaugeAllocatedForPeriod[gauge][period] = true;

        // ...

      }

Although this weird state doesn't have high likelyhood, it would impact the _createLoot() function, allowing all users to create their loots twice (one with each distributor). The only pre-condition for this exploit, is that the distributor also calls LootCreator::notifyQuestClaim(), making userQuestPeriodRewards be non zero, so that the following check passes in _createLoot():

      function _createLoot(address user, address distributor, uint256 questId, uint256 period) internal {
        // ...

@>      vars.userPeriodRewards = userQuestPeriodRewards[distributor][questId][period][user];
@>      if(vars.userPeriodRewards == 0) return;

        // ...

I consider this pre-requisite a side-effect of the previous condition: if two distributors call notifyDistributedQuestPeriod() it is reasonable to think that they would also call notifyQuestClaim(), as it is part of the same task/responsability of a distributor.

Another impact of less severity of this issue is that the view function getQuestAllocationForPeriod() reads from _getQuestAllocationForPeriod() which calculates the allocation based on the double set totalQuestPeriodRewards. Hence, the view function would show allocation for both distributors for the same questId and period.

Attack Scenario

• Distributor1 calls notifyDistributedQuestPeriod() for a given questId and period
• Distributor1 calls notifyQuestClaim() when the user claims the period
• Distributor2 calls notifyDistributedQuestPeriod() for the same questId and period
• Distributor2 calls notifyQuestClaim() when the user claims the period
• User calls createLoot() with distributor=Distributor1
• User calls createLoot() with distributor=Distributor2 (double creation)

Impact

• Likelyhood: low, as it requires that two distributors handle the same quest & period.
• Impact:
  • If the above attack scenario is a valid path, the impact would be high, as it would allow all users to double-create loots. In that case, the overall severity of the issue is medium.
  • If the avove attack scenario is not a valid path, the impact would be low, as it only affects view functions. In that case the overall severity would be low.

Recommendation Perform the check with isGaugeAllocatedForPeriod before the totalRewards can be allocated to totalQuestPeriodRewards for the msg.sender:

    function notifyDistributedQuestPeriod(uint256 questId, uint256 period, uint256 totalRewards) external onlyAllowedDistributor nonReentrant {
        // Pull any new budget & update the current period to have an up to date budget
        _pullBudget();
        _updatePeriod();

        // Fetch the gauge for the quest & check if it's listed
        address gauge = _getQuestGauge(questId, msg.sender);
        if(!ILootVoteController(lootVoteController).isListedGauge(gauge)) return;

+       // If the period is already allocated for the gauge, return
+       if(isGaugeAllocatedForPeriod[gauge][period]) return;
+       isGaugeAllocatedForPeriod[gauge][period] = true;

        // If not set yet, set the total rewards for the quest & period
        if(!totalQuestPeriodSet[msg.sender][questId][period]) {
            totalQuestPeriodRewards[msg.sender][questId][period] = totalRewards;
            totalQuestPeriodSet[msg.sender][questId][period] = true;
        }

-       // If the period is already allocated for the gauge, return
-       if(isGaugeAllocatedForPeriod[gauge][period]) return;
-       isGaugeAllocatedForPeriod[gauge][period] = true;

        // ...

[PlamenTSV]

If the total rewards for the quest and period are already set nothing happens, wherever you put the if check. I kind of don't get this.

[JacoboLansac]

The the total rewards are set for the quest, period AND msg.sender (the distributor).

When notifyDistributedQuestPeriod is called, the totalQuestPeriodSet[msg.sender][questId][period] is set to true only for the msg.sender, (which can only be a distributor due to the onlyAllowedDistributor modifier). If a second distributor calls this function afterwards, the following check will be false, as it is a different msg.sender:

if(!totalQuestPeriodSet[msg.sender][questId][period]) { ...

So, after two distributors (distributorA, distributorB) call this function, the state will be: totalQuestPeriodRewards[distributorA][questId][period] > 0 totalQuestPeriodRewards[distributorB][questId][period] > 0

Let me know if it makes sense

[Kogaroshi]

This is invalid, as the scenario you describe as a wrong double creation is a normal behavior. You are talking about 2 different Distributors, which means each Distributor has its own QuestBoard, where the questIds are internal to the QuestBoard own storage. So if indeed 2 Distributors notify for a questId of 3 for the same period, it will be 2 different Quests, with different gauges.

[JacoboLansac]

I see. I understand your point.

However, what you describe is not implicit in the code of any of the in-scope contracts, as it is part of the QuestBoard contract (which not in scope, and not even developed as far as I understood).

The contracts in scope do allow two different distributors to call notifyDistributedQuestPeriod() potentially creating this invalid blockchain state (that would only affect the view functions). So I believe it should be considered a valid low severity bug.

[Kogaroshi]

QuestBoard contracts are deployed and live (for a while now), and can be found in the Paladin documentation. And as for any security audit, it's also the responsibility of the researcher to do their research on the potential external interactions. And even without taking the QuestBoard in mind here, the attack scenario you describe is still invalid, as the questIds and period are stored based on each Distributor, and you take the assumption that the gauge matching each questId is gonna be the same in your scenario, but this information comes from the QuestBoard once again, which would mean you didn't mind to research enough how the questIds are matched with gauge addresses, which is why I stated it as invalid.

[JacoboLansac]

You are right. I just saw that the QuestBoad is indeed in the documentation, and I missed it. Sorry about that. My bad. I agree that the bug is invalid.

Regarding the last part of your message ... I understand you are probably tired of discussing with auditors about invalid bugs, but there is no need to be disrespectful. It is not that "I didn't mind researching enough". I did mind, and I think I am doing my best in understanding your full system in a very short period of time, and I simply missed the match of the questIds and gauges.

No hard feelings though. Let's just focus on what matters while keeping a healthy environment. Your codebase is really well thought out in terms of security and it is a pleasure to work with such a system. Keep up the good work