berndartmueller - Draw auction rewards likely exceed the available rewards, resulting in overpaying rewards or running into an `InsufficientReserve` error

[sherlock-admin2]

berndartmueller

high

Draw auction rewards likely exceed the available rewards, resulting in overpaying rewards or running into an InsufficientReserve error

Summary

The sum of the calculated startDraw and finishDraw reward fractions likely exceeds 1.0, which can lead to overpaying rewards or an InsufficientReserve error.

Vulnerability Detail

The finishDraw function calculates the rewards for starting and finishing a draw and distributes them to the respective participants. The total rewards (availableRewards) are based on the available prize pool reserve (upper capped by maxRewards). Those rewards are then split into the start draw rewards and the finish draw rewards. Internally, the portion each participant receives is denominated in fractions ([0.00, 1.00]). No more than availableRewards (i.e., the upper cap or the total available prize pool reserve)can be distributed.

However, it is very likely that the sum of all fractions exceeds 1.0, which would result in utilizing more rewards than availableRewards. Consequently, if the reserve has been larger than the maxRewards upper cap, more rewards would be paid out than availableRewards. Or, if the reserve has been smaller than maxRewards, which means the entire reserve is supposed to get used for rewards, using more than 1.0 * availableRewards would result in an InsufficientReserve error.

Basically, in the former case, rewards are overpaid, and in the latter case, the draw can not be awarded due to the InsufficientReserve error.

The following test case demonstrates this issue by having two startDraw calls, both of which are supposed to receive ~0.1e18 rewards, which would already exceed the 0.1e18 available reserve. Please note that due to mocking the calls to the prize pool, the actual InsufficientReserve error is not triggered. However, by inspecting the emitted events, it can be seen that the rewards are overpaid.

function test_finishDraw_multipleStarts_with_exceeding_reserve() public {
    vm.warp(1 days + auctionDuration);

    mockReserve(.1e18, 0);

    // 1st start draw
    mockRng(99, 0x1234);
    uint firstStartReward = 99999999999999999; // ~0.1e18
    vm.expectEmit(true, true, true, true);
    emit DrawStarted(address(this), alice, 1, auctionDuration, firstStartReward, 99, 1);
    drawManager.startDraw(alice, 99);

    // RNG fails
    mockRngFailure(99, true);

    // 2nd start draw
    vm.warp(1 days + auctionDuration + auctionDuration);
    vm.roll(block.number + 1);
    mockRng(100, 0x1234);
    uint secondStartReward = 99999999999999999; // ~0.1e18 - Would already exceed the available reserve of 0.1e18
    vm.expectEmit(true, true, true, true);
    emit DrawStarted(address(this), bob, 1, auctionDuration, secondStartReward, 100, 2);
    drawManager.startDraw(bob, 100);

    mockFinishDraw(0x1234);
    vm.warp(1 days + auctionDuration*3);
    uint finishReward = 99999999999999999; // ~0.1e18
    mockAllocateRewardFromReserve(alice, firstStartReward);
    mockAllocateRewardFromReserve(bob, secondStartReward);
    mockAllocateRewardFromReserve(bob, finishReward);
    mockReserveContribution(.1e18);
    assertEq(drawManager.finishDrawReward(), finishReward, "finish draw reward matches");
    drawManager.finishDraw(bob);
}

Please copy and paste the above test case into the pt-v5-draw-manager/test/DrawManager.t.sol file and run it with forge test -vv --match-test "test_finishDraw_multipleStarts_with_exceeding_reserve".

Impact

Either rewards for startDraw and finishDraw are overpaid, or the draw can not be awarded.

Code Snippet

DrawManager.finishDraw#L314-L352

314: function finishDraw(address _rewardRecipient) external returns (uint24) {
...    // [...]
333:   uint256 availableRewards = _computeAvailableRewards();
334:   (uint256[] memory startDrawRewards, UD2x18[] memory startDrawFractions) = _computeStartDrawRewards(
335:     prizePool.drawClosesAt(startDrawAuction.drawId),
336:     availableRewards
337:   );
338:   (uint256 _finishDrawReward, UD2x18 finishFraction) = _computeFinishDrawReward(
339:     startDrawAuction.closedAt,
340:     block.timestamp,
341:     availableRewards
342:   );
343:   uint256 randomNumber = rng.randomNumber(startDrawAuction.rngRequestId);
344:   uint24 drawId = prizePool.awardDraw(randomNumber);
345:
346:   lastStartDrawFraction = startDrawFractions[startDrawFractions.length - 1];
347:   lastFinishDrawFraction = finishFraction;
348:
349:   for (uint256 i = 0; i < _startDrawAuctions.length; i++) {
350:     _reward(_startDrawAuctions[i].recipient, startDrawRewards[i]);
351:   }
352:   _reward(_rewardRecipient, _finishDrawReward);

Tool used

Manual Review

Recommendation

Consider ensuring that the sum of all fractions does not exceed 1.0 to prevent overpaying rewards or running into an InsufficientReserve error.

[trmid]

The suggested mitigation is a good addition to the draw auction logic, however the difference in behaviour between the fix and the current behaviour is minimal:

Current Behaviour:

When there is not enough reserve to pay for the finishDraw, the draw will not be awarded and the prize liquidity will gracefully roll over to the next draw (the prize pool is well-equipped to handle skipped draws). The only loss here is on the agent that started the draw, who will not receive the rewards for doing so.

Behaviour After Fix

When there is not enough reserve to pay for the finishDraw, the auction price will remain at the value of whatever is left until the end of the auction incase a benevolent actor decides to push it through at a loss. If no one pushes it through, the draw will be skipped the same as before and the agent that started the draw will still receive no rewards.

The only difference is the amount of time that a benevolent actor has to push the draw through at a loss. No core protocol functionality is broken since the prize pool is only expected to award prizes if there are enough incentives to do so, and it's built to handle skipped draws without loss.

[nevillehuang]

@trmid I see users that was supposed to be awarded in the original draw as losing their funds as well (since they lose their chance to win), not just the agent that started the draw.

Was this behavior documented as expected behavior? If not I would lean towards agreeing that high severity is appropriate.

[trmid]

A severity of high seems inappropriate for this issue.

The prize pool uses part of the yield generation (the reserve) to incentivize the RNG auctions. If the generated value is insufficient for a draw to be awarded, then the prize pool keeps all the prize liquidity for the draw and includes it in the next draw. The issue mentioned here is not something that can be forced or exploited, rather it is describing a better way to handle the auction pricing when there is less yield than required to incentivize the RNG. The only difference between the current behaviour and the suggested mitigation behaviour is that the auction will be available for a longer period of time when it does not have proper liquidity for a profitable payout.

This issue is assuming that there are not enough incentives for the RNG auctions to provide profitable payouts, therefore medium seems more appropriate:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/GenerationSoftware/pt-v5-draw-manager/pull/18

[10xhash]

Fixed with https://github.com/GenerationSoftware/pt-v5-draw-manager/pull/17 and https://github.com/GenerationSoftware/pt-v5-draw-manager/pull/18 Now the max reward is limited to remaining fraction after the other rewards are considered

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.