ydlee - For a new `drawId`, `startDraw` always reverts if the time elapsed for the first `startDraw` exceeds the auction duration.

[sherlock-admin3]

ydlee

medium

For a new drawId, startDraw always reverts if the time elapsed for the first startDraw exceeds the auction duration.

Summary

For a new drawId, if the time elapsed for the first startDraw exceeds the auction duration, the first startDraw will reverts with AuctionExpired, and subsequent startDraw attempts for the drawId will also revert, making the startDraw auction for this drawId unsuccessful.

Vulnerability Detail

The first startDraw auction for a new drawId opens when the draw closes (i.e. auctionOpenedAt = closesAt, L232). If the time elapsed for the first startDraw exceeds the auction duration, this startDraw will revert with AuctionExpired (L251), and will NOT be stored into _startDrawAuctions (L253). In such case, subsequent startDraw attempts for this drawId will also revert because their auctionOpenedAt are always the closesAt time of the drawId (L232), and the time elapsed will always be greater than that of the first startDraw, thus exceeding the auction duration and reverting. This makes the retries meaningless and thus breaks the retry mechanism of startDraw.

219:  function startDraw(address _rewardRecipient, uint32 _rngRequestId) external returns (uint24) {
220:
221:    if (_rewardRecipient == address(0)) revert RewardRecipientIsZero();
222:    uint24 drawId = prizePool.getDrawIdToAward(); 
223:    uint48 closesAt = prizePool.drawClosesAt(drawId);
224:    if (closesAt > block.timestamp) revert DrawHasNotClosed();
225:    if (rng.requestedAtBlock(_rngRequestId) != block.number) revert RngRequestNotInSameBlock();
226:    
227:    StartDrawAuction memory lastRequest = getLastStartDrawAuction();
228:    uint256 auctionOpenedAt;
229:    
230:    if (lastRequest.drawId != drawId) { // if this request is for a new draw
231:      // auctioned opened at the close of the draw
232:@>    auctionOpenedAt = closesAt;
233:      // clear out the old ones
234:      while (_startDrawAuctions.length > 0) {
235:        _startDrawAuctions.pop();
236:      }
237:    } else { // the old request is for the same draw
238:      if (!rng.isRequestFailed(lastRequest.rngRequestId)) { // if the request failed
239:        revert AlreadyStartedDraw();
240:      } else if (_startDrawAuctions.length > maxRetries) { // if request has failed and we have retried too many times
241:        revert RetryLimitReached();
242:      } else if (block.number == rng.requestedAtBlock(lastRequest.rngRequestId)) { // requests cannot be reused
243:        revert StaleRngRequest();
244:      } else {
245:        // auctioned opened at the close of the last auction
246:        auctionOpenedAt = lastRequest.closedAt;
247:      }
248:    }
249:
250:    uint48 auctionElapsedTimeSeconds = _computeElapsedTime(auctionOpenedAt, block.timestamp);
251:@>  if (auctionElapsedTimeSeconds > auctionDuration) revert AuctionExpired();
252:
253:@>  _startDrawAuctions.push(StartDrawAuction({
254:      recipient: _rewardRecipient,
255:      closedAt: uint40(block.timestamp),
256:      drawId: drawId,
257:      rngRequestId: _rngRequestId
258:    }));

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-draw-manager/src/DrawManager.sol#L219-L258

Impact

If the first startDraw auction expires, subsequent startDraw attempts will also expire. This makes the startDraw auction for a drawId fail, breaking the retry mechanism of startDraw.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-draw-manager/src/DrawManager.sol#L219-L258

Tool used

Manual Review

Recommendation

// 1. First add a storage variable to record the time at which the last auction closed.
    mapping(uint24 drawId => uint256 lastAuctionClosedAt) lastStartDrawClosedAt;

// 2. Then moidify function startDraw() as follows:
     if (lastRequest.drawId != drawId) { // if this request is for a new draw
-      // auctioned opened at the close of the draw
-      auctionOpenedAt = closesAt;
-      // clear out the old ones
-      while (_startDrawAuctions.length > 0) {
-        _startDrawAuctions.pop();
-      }
+      if (lastStartDrawClosedAt[drawId] == 0) {
+        // auctioned opened at the close of the draw
+        auctionOpenedAt = closesAt;
+        // clear out the old ones
+        while (_startDrawAuctions.length > 0) {
+            _startDrawAuctions.pop();
+        }
+      } else {
+        auctionOpenedAt = lastStartDrawClosedAt[drawId];
+      }
     } else { // the old request is for the same draw
       if (!rng.isRequestFailed(lastRequest.rngRequestId)) { // if the request failed
         revert AlreadyStartedDraw();
       } else if (_startDrawAuctions.length > maxRetries) { // if request has failed and we have retried too many times
         revert RetryLimitReached();
       } else if (block.number == rng.requestedAtBlock(lastRequest.rngRequestId)) { // requests cannot be reused
         revert StaleRngRequest();
       } else {
         // auctioned opened at the close of the last auction
         auctionOpenedAt = lastRequest.closedAt;
       }
     }

+    lastStartDrawClosedAt[drawId] = block.timestamp;

     uint48 auctionElapsedTimeSeconds = _computeElapsedTime(auctionOpenedAt, block.timestamp);
     if (auctionElapsedTimeSeconds > auctionDuration) revert AuctionExpired();

Duplicate of #129