Incorrect Allocation results in `getQuestAllocationForPeriod` when `questRewardToken` has Decimals other than 18

[hats-bug-reporter[bot]]

Github username: @chainNue Twitter username: chainNue Submission hash (on-chain): 0xc24d2916bc90d76389dce246dac38acc72f5be3dfcd458783a75eb858ad13273 Severity: medium

Description: Description

In Distributor contract, questBoard can addQuest with prefered questId and a reward token. Looking on several sources, this reward token could be a USDC 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48:

• on Quest create page, there is option to choose USDC as reward token
• on deployed QuestBoard, the USDC is one of whitelisted reward token

The USDC token is using 6 decimals, unlike DAI 18 decimals.

File: MultiMerkleDistributorV2.sol
291:     function addQuest(uint256 questID, address token) external returns(bool) {
...
296:         // Add a new Quest using the QuestID, and list the reward token for that Quest
297:         questRewardToken[questID] = token;
298:
299:         if(!rewardTokens[token]) rewardTokens[token] = true;
...
304:     }

When updateQuestPeriod is executed, notifyDistributedQuestPeriod on LootCreator will be called passing the totalAmount which is the questRewardsPerPeriod, (6 decimals)

On LootCreator this amount will be saved into totalQuestPeriodRewards. Next when calculating allocation (Line 411), it will passed as questTotalRewards.

Here, in _getQuestAllocationForPeriod, the calculation will face a conversion issue, since the UNIT conversion is 1e18 and not considering the reward decimal.

File: LootCreator.sol
403:     function _getQuestAllocationForPeriod(
404:         address gauge,
405:         uint256 questId,
406:         address distributor,
407:         uint256 period
408:     ) internal view returns(Allocation memory) {
...
411:         uint256 questTotalRewards = totalQuestPeriodRewards[distributor][questId][period];
...
421:         if(nbQuestForGauge == 1) {
422:             allocation.palPerVote = uint128((((budget.palAmount * UNIT) / questTotalRewards) * UNIT) / MAX_MULTIPLIER);
423:             allocation.extraPerVote = uint128((((budget.extraAmount * UNIT) / questTotalRewards) * UNIT) / MAX_MULTIPLIER);
...
430:     }

Reading Line 422, allocation.palPerVote = uint128((((budget.palAmount * UNIT) / questTotalRewards) * UNIT) / MAX_MULTIPLIER);

the allocation palPerVote (and extraPerVote) will have issue, because:

• palAmount = 18 decimals
• UNIT = 18 decimals
• questTotalRewards = 6 decimals
• MAX_MULTIPLIER = 18 decimals

Resulting, palPerVote is in order of (((18+18) - 6) + 18) - 18 = 30 decimals.

Attack Scenario

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation

Perform a conversion factor based on the difference in decimals, for example uint256 conversionFactor = 10**(18 - tokenDecimals);

[PlamenTSV]

The conversion is done to avoid precision loss. You have multiplication by UNIT in the numerator AND the denominator, which cancel each other out. If you track the contract flow, you will see that every multiplication by UNIT eventually gets divided by UNIT. Imo the 1e18 is perfect since it avoids both precision loss and silent overflow.

[chainNue]

if the contract flow mean up until the end of _createLootthen yes, it is in correct result. but in _getQuestAllocationForPeriod (as there is an external view getQuestAllocationForPeriod) the result is wrong. My bad is, I set it to medium instead of Low.

what I mean is this following function will return incorrect result:

File: LootCreator.sol
218:     function getQuestAllocationForPeriod(
219:         uint256 questId,
220:         address distributor,
221:         uint256 period
222:     ) external view returns(uint256 palPerVote, uint256 extraPerVote) {
223:         address gauge = _getQuestGauge(questId, distributor);
224:         Allocation memory allocation = _getQuestAllocationForPeriod(gauge, questId, distributor, period);
225:         palPerVote = allocation.palPerVote;
226:         extraPerVote = allocation.extraPerVote;
227:     }

to make it clear, the palPerVote calculation

allocation.palPerVote = uint128((((budget.palAmount * UNIT) / questTotalRewards) * UNIT) / MAX_MULTIPLIER);

For example if:

• palAmount = 20e18
• UNIT = 1e18
• questTotalRewards = 2e6
• MAX_MULTIPLIER = 5e18

Calculation:

1. Multiply palAmount and UNIT: intermediateResult1 = palAmount * UNIT = (20 * 1e18) * (1e18) = 20e36

2. Divide by questTotalRewards: intermediateResult2 = intermediateResult1 / questTotalRewards = 20e36 / 2e6 = 10e30

3. Multiply by UNIT again: intermediateResult3 = intermediateResult2 * UNIT = 10e30 * (1e18) = 10e48

4. Divide by MAX_MULTIPLIER: palPerVote = intermediateResult3 / MAX_MULTIPLIER = 10e48 / (5e18) = 2e30

Therefore, palPerVote = 2e30 , or in other word palPerVote is 2e12 * 1e18, the allocation of 2e12 PAL per vote seems off to me.

(assume rewardDecimals = 6 decimal)
uint256 conversionFactor = 10**(18 - rewardDecimals); // but if decimal = 18, keep conversion 1e18
allocation.palPerVote = uint128((((budget.palAmount * conversionFactor) / questTotalRewards) * conversionFactor) / MAX_MULTIPLIER);

then the _createLoot should also be fixed up to make up for this adjustment. for instance, vars.userPalAmount = ((uint256(allocation.palPerVote) * vars.userMultiplier / UNIT) * vars.userPeriodRewards / UNIT); should be vars.userPalAmount = ((uint256(allocation.palPerVote) * vars.userMultiplier / conversionFactor) * vars.userPeriodRewards / conversionFactor);

[Kogaroshi]

Should we change in the _createLoot() function, which would increase gas costs for this method (and so for each creation in the case of createMultipleLoot()), and potential precision loss. Instead I think we can see to make that conversion if needed in the view function getQuestAllocationForPeriod() so the result returned are correct on a 18 decimal base, but do not impact the flow of the internal functions.

[chainNue]

as a quick fix yes, it's reasonable to make conversion only on the view function. but in the _createLoot, internally the allocation values might be wrong, which is not a good practice keeping this state. Also, yes agree, precision loss and overflow issue should be considered seriously in this _createLoot, so a quick patch is not preferable.

[Kogaroshi]

Change made in https://github.com/PaladinFinance/Vote-Flywheel/pull/2/commits/c91106a717a2a01cf4c6c82ae5cde9299222d0d0 Handling all cases of non 18 decimals tokens.

[blueskyHope507]

@Kogaroshi , this issue is at most low as submitter agreed. Everything worked correctly and all calculations were correct when creating Loot. At worst, this is a small issue in view function. This kind of issue has been treated as low/information in other platforms.