berndartmueller - The RNG finish draw auction rewards are overpaid due to missing to account for the time it takes to fulfill the Witnet randomness request

[sherlock-admin3]

berndartmueller

medium

The RNG finish draw auction rewards are overpaid due to missing to account for the time it takes to fulfill the Witnet randomness request

Summary

The rewards for finishing the draw and submitting the previously requested randomness result are slightly overpaid due to the incorrect calculation of the elapsed auction time, wrongly including the time it takes to fulfill the Witnet randomness request.

Vulnerability Detail

The DrawManager.finishDraw function calculates the rewards for finishing the draw, i.e., providing the previously requested randomness result, via the _computeFinishDrawReward function in lines 338-342. The calculation is based on the difference between the timestamp when the start draw auction was closed (startDrawAuction.closedAt) and the current block timestamp. Specifically, a parabolic fractional dutch auction (PFDA) is used to incentivize shorter auction durations, resulting in faster randomness submissions.

However, the requested randomness result at block X is not immediately available to be used in the finishDraw function. According to the Witnet documentation, the randomness result is available after 5-10 minutes. This time delay is currently included when determining the elapsed auction time because startDrawAuction.closedAt marks the time when the randomness request was made, not when the result was available to be submitted.

Consequently, the protocol always overpays rewards for the finish draw. Over the course of many draws, this can lead to a significant overpayment of rewards.

It should be noted that the PoolTogether documentation about the draw auction timing clearly outlines the timeline for the randomness auction and states that finish draw auction price will rise once the random number is available:

The RNG auction for any given draw starts at the beginning of the following draw period and must be completed within the auction duration. Following the start RNG auction, there is an “unknown” waiting period while the RNG service fulfills the request.

Once the random number is available, the finished RNG auction will rise in price until it is called to award the draw with the available random number. Once the draw is awarded for a prize pool, it will distribute the fractional reserve portions based on the auction results that contributed to the closing of that draw.

Impact

The protocol overpays rewards for the finish draw.

Code Snippet

pt-v5-draw-manager/src/DrawManager.sol#L339

311: /// @notice Called to award the prize pool and pay out rewards.
312: /// @param _rewardRecipient The recipient of the finish draw reward.
313: /// @return The awarded draw ID
314: function finishDraw(address _rewardRecipient) external returns (uint24) {
315:   if (_rewardRecipient == address(0)) {
316:     revert RewardRecipientIsZero();
317:   }
318:
319:   StartDrawAuction memory startDrawAuction = getLastStartDrawAuction();
...    // [...]
338:   (uint256 _finishDrawReward, UD2x18 finishFraction) = _computeFinishDrawReward(
339: ❌  startDrawAuction.closedAt,
340:     block.timestamp,
341:     availableRewards
342:   );
343:   uint256 randomNumber = rng.randomNumber(startDrawAuction.rngRequestId);

Tool used

Manual Review

Recommendation

Consider determining the auction start for the finish draw reward calculation based on the timestamp when the randomness result was made available. This timestamp is accessible by using the WitnetRandomnessV2.fetchRandomnessAfterProof function (instead of fetchRandomnessAfter). The _witnetResultTimestamp return value can be used to better approximate the auction start, hence, paying out rewards more accurately.

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

this is exactly the _firstFinishDrawTargetFraction goal

[trmid]

The suggested mitigation is flawed since it would return the timestamp that the RNG was generated on the Witnet network, not the timestamp that it was relayed to the requesting network.

The auction should be set with an appropriate _firstFinishDrawTargetFraction _auctionTargetTime value such that the estimated auction value is reached after the 5-10 min expected relay time. In addition, the maxRewards parameter on the DrawManager contract helps to ensure a reasonable max limit to the total auction payout in the case of an unexpected outage.

[nevillehuang]

@berndartmueller Could you take a look at the above comment? Does an appropriately admin set _firstFinishDrawTargetFraction mitigate this issue?

[berndartmueller]

Hey @nevillehuang!

The suggested mitigation is flawed since it would return the timestamp that the RNG was generated on the Witnet network, not the timestamp that it was relayed to the requesting network.

This statement is mostly true, but, the timestamp can also be the block.timestamp at the time when the RNG request was relayed to the requesting network. Specifically, when reporters use the reportResult function, the timestamp is set to block.timestamp.

If reporters use one of the two other reporting methods, the timestamp is set to the time when the RNG was generated on the Witnet network (although it's not verified that reporters set this value exactly, the timestamp could also be set to block.timestamp). To be fair, on-chain Optimism transactions to the Witnet contract show that the reportResultBatch function is predominantly used.

However, reporters are free to use whatever relaying function they use, so it's possible that the timestamp is the time when the request was made available to the requesting network. Either way, IMO, this makes it a better approximation of when the random number is available to PoolTogether's finishDraw, than using the time when startDraw was called and the RNG requested (which neglects the time it needs for Witnet actors to relay the request to the Witnet chain, generate the random number and relay it back to the requesting chain)

_The auction should be set with an appropriate firstFinishDrawTargetFraction value such that the estimated auction value is reached after the 5-10 min expected relay time. In addition, the maxRewards parameter on the DrawManager contract helps to ensure a reasonable max limit to the total auction payout in the case of an unexpected outage.

_firstFinishDrawTargetFraction is provided once in the constructor and stored in lastFinishDrawFraction. And lastFinishDrawFraction is updated in every finishDraw call. While it's true that this value is used to better estimate the expected rewards and the relay time, it's also not perfect. A single occurrence of a longer than usual RNG relay time is enough to set lastFinishDrawFraction to an outlier value (i.e., inflated value, but upper-capped by maxRewards).

Long story short, using the recommendation in this submission, in conjunction with providing a reasonable _firstFinishDrawTargetFraction value by the admin, further improves the accuracy of the rewards calculation (i.e., prevents overpaying).

[trmid]

Removed the "disputed" tag since this seems like a valid issue for the auctions, but may not need additional mitigation.

I realized I quoted the wrong parameter in my statement above:

The auction should be set with an appropriate _firstFinishDrawTargetFraction value such that the estimated auction value is reached after the 5-10 min expected relay time.

I meant to say _auctionTargetTime here instead of _firstFinishDrawTargetFraction.