sherlock-audit / 2024-05-pooltogether-judging

2 stars 0 forks source link

infect3d - `DrawManager::finishDraw` can revert when the sum of rewards to distribute is more than available reserve #123

Closed sherlock-admin3 closed 1 month ago

sherlock-admin3 commented 1 month ago

infect3d

medium

DrawManager::finishDraw can revert when the sum of rewards to distribute is more than available reserve

Summary

For each draw, depending on how the auction is concluded, the drawers will receive a fraction of the reserve. If the sum of all fractions to distribute is greater than 100%, then finishDraw, which is responsible for calling the PrizePool::awardDraw will revert, preventing the draw to be awarded with no possibility for the function to recover until next draw.

Vulnerability Detail

To be awarded, the draw needs two things to happen: 1) a random number must be generated: this is achieved by calling RngWitnet::startDraw() which itself calls DrawManager::startDraw(). This step take some time as the random number (RN) generation is executed by the Witnet blockchain, which return the value to the requesting chain after few minutes. This function cannot be called until next draw starts, or RNG fails. 2) once the RN is returned by the oracle, finishDraw must be called by a drawer to send it to the Prize Pool so that prizes can be awarded. This is during that call that all reward are computed and awarded to drawers.

The protocol incentivize drawers by running a PFDA auction, where the reward allocated for doing one of these 2 actions increase over time. The actual implemented PFDA algorithm do not return a price, but returns a fraction of the total reserve that will be attributed to the drawer.

As we said in (1), if the RN generation fails, anyone can call again startDraw() to run a new RNG. Once a valid RN is returned, all drawers that participated in the generation (failed generation and successful) will receive the expected reward, as re-running the function will add the drawer to an array to be processed afterward for reward distribution. The maximum number of try is capped by maxRetries. So, there can be up to maxRetries startDrawRewards, and one finishDrawReward.

The issue is that the computation of the fraction allocated to each reward is done independentely to the other already planned reward fractions, and computed solely on the timing of the auction.

We can infer that when (a) startDraw auctions are done more than once and (b) when auctions are completed "late", the sum of the returned fractions by the PFDA can be greater than 1. If this happens, when reward will be distributed through _reward(), which calls PrizePool::allocateReserveFromReserve will revert for the reward that try to allocate more than the available _reserve (as previous allocated rewards reduced it)

Impact

The draw cannot be completed and will be discarded, thus disrupting the awarding process and prize distribution.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether//blob/main/pt-v5-draw-manager/src/DrawManager.sol#L105 https://github.com/sherlock-audit/2024-05-pooltogether//blob/main/pt-v5-draw-manager/src/DrawManager.sol#L349-L352 https://github.com/sherlock-audit/2024-05-pooltogether//blob/main/pt-v5-prize-pool/src/PrizePool.sol#L438-L440

Tool used

Manual review

Recommendation

Don't allow rewards to be greater than available reserve. This can be done either by saving the remaining reserve each time a startDraw has been conducted and reward calculated, or by checking that the sum of the fraction is less than 1, and if not the case, normalizing the fractions such as the sum is <=1.

Duplicate of #125