DrawManager::finishDraw() may hand out more rewards than the reserve if RNG requests fail
Summary
DrawManager::finishDraw() calculates reward fractions for each tried auction within a draw, but does not verify if the total fraction is bigger than the reserve, leading to the failure of some draws.
Vulnerability Detail
DrawManager::startDraw() is the entrypoint to start draw actions and hands out rewards according to the curve in the docs. The target reward fraction is a fraction of the reserve, so a number between 0 and 1. Considering for example that the start reward fraction is 0.25, the finish fraction (there is an auction to finish the draw by calling DrawManager::finishDraw()) is 0.25 and there are 2 retries, if the first 2 requests fail but the third succeeds, the reward fraction will exceed 1 and DrawManager::finishDraw() will revert due to withdrawing more from the reserve than possible.
For example, in the scenario above, assuming the draws are executed slightly later than the target time, the total reward fraction is 0.251 + 0.251 + 0.251 + 0.251 = 1.004, so it tries to fetch more from the reserve than possible and reverts in PrizePool::allocateRewardFromReserve(),
if (_amount > _reserve) {
revert InsufficientReserve(_amount, _reserve);
}
This is less likely to happen if the reserve is bigger than maxRewards in the DrawManager, but even then it may still happen. If the reserve is smaller than maxRewards, it is more likely.
Impact
Failed draws due to not limiting the rewards fraction to 1, as it underflows in PrizePool::allocateRewardFromReserve().
Limit the reward fraction to 1, for example by dividing pro-rata to the excess of the total reward fraction. For example, if the total is 1.1, each reward fraction may be decreased 1.1 times to not make the draw revert.
0x73696d616f
medium
DrawManager::finishDraw()may hand out more rewards than the reserve if RNG requests failSummary
DrawManager::finishDraw()calculates reward fractions for each tried auction within a draw, but does not verify if the total fraction is bigger than the reserve, leading to the failure of some draws.Vulnerability Detail
DrawManager::startDraw()is the entrypoint to start draw actions and hands out rewards according to the curve in the docs. The target reward fraction is a fraction of the reserve, so a number between 0 and 1. Considering for example that the start reward fraction is 0.25, the finish fraction (there is an auction to finish the draw by callingDrawManager::finishDraw()) is 0.25 and there are 2 retries, if the first 2 requests fail but the third succeeds, the reward fraction will exceed 1 andDrawManager::finishDraw()will revert due to withdrawing more from the reserve than possible. For example, in the scenario above, assuming the draws are executed slightly later than the target time, the total reward fraction is 0.251 + 0.251 + 0.251 + 0.251 = 1.004, so it tries to fetch more from the reserve than possible and reverts inPrizePool::allocateRewardFromReserve(),This is less likely to happen if the reserve is bigger than
maxRewardsin theDrawManager, but even then it may still happen. If the reserve is smaller thanmaxRewards, it is more likely.Impact
Failed draws due to not limiting the rewards fraction to 1, as it underflows in
PrizePool::allocateRewardFromReserve().Code Snippet
https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-draw-manager/src/DrawManager.sol#L237 https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-draw-manager/src/DrawManager.sol#L454 https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-draw-manager/src/DrawManager.sol#L471
Tool used
Manual Review
Vscode
Recommendation
Limit the reward fraction to
1, for example by dividing pro-rata to the excess of the total reward fraction. For example, if the total is 1.1, each reward fraction may be decreased 1.1 times to not make the draw revert.Duplicate of #125