MiloTruck - Draws can be retried even if a random number is available or the current draw has finished

[sherlock-admin3]

MiloTruck

medium

Draws can be retried even if a random number is available or the current draw has finished

Summary

RngWitnet.isRequestFailed() does not accurately represent whether a random number is available, making it possible for startDraw() to be called in conditions that causes a loss of funds for draw bots.

Vulnerability Detail

RngWitnet.isRequestFailed() determines if a request has failed by checking the response status of a specific block number is equal to WitnetV2.ResponseStatus.Error:

RngWitnet.sol#L115-L118

    function isRequestFailed(uint32 _requestId) onlyValidRequest(_requestId) public view returns (bool) {
        (uint256 witnetQueryId,,) = witnetRandomness.getRandomizeData(requests[_requestId]);
        return witnetRandomness.witnet().getQueryResponseStatus(witnetQueryId) == WitnetV2.ResponseStatus.Error;
    }

Note that requests[_requestId], stores the block number at which the request at _requestId was made.

However, isRequestFailed() does not accurately reflect if a random number is available. In Witnet, isRandomized() (which is used in isRequestComplete()) returns true and fetchRandomnessAfter() (which is used in randomNumber()) returns a valid random number as long as there is a successful request at or after the requested block number:

WitnetRandomnessV2.sol#L334-L336

    /// @notice Returns `true` only if a successfull resolution from the Witnet blockchain is found for the first 
    /// @notice non-errored randomize request posted on or after the given block number.
    function isRandomized(uint256 _blockNumber)

WitnetRandomnessV2.sol#L128-L136

    /// @notice Retrieves the result of keccak256-hashing the given block number with the randomness value 
    /// @notice generated by the Witnet Oracle blockchain in response to the first non-errored randomize request solved 
    /// @notice after such block number.
    /// @dev Reverts if:
    /// @dev   i.   no `randomize()` was requested on neither the given block, nor afterwards.
    /// @dev   ii.  the first non-errored `randomize()` request found on or after the given block is not solved yet.
    /// @dev   iii. all `randomize()` requests that took place on or after the given block were solved with errors.
    /// @param _blockNumber Block number from which the search will start
    function fetchRandomnessAfter(uint256 _blockNumber)

For example, assume there are two requests:

• Request 1 made at block.number = 100 failed.
• Request 2 made at block.number = 101 was successful.

If isRandomized() and fetchRandomnessAfter() was called for block number 100, they would return true and a valid random number respectively. By extension, RngWitnet.isRequestComplete() would return true for request 1. However, RngWitnet.isRequestFailed() also returns true for request 1, forming a contradiction.

As such, it is possible for RngWitnet.isRequestFailed() to return true for a given _requestId, even when RngWitnet.isRequestComplete() returns true and RngWitnet.randomNumber() returns a valid random number.

In DrawManager.startDraw(), if RngWitnet.isRequestFailed() returns true, draw bots are allowed to call startDraw() again to submit a new request for the current draw to "retry" the randomness request:

DrawManager.sol#L238-L239

      if (!rng.isRequestFailed(lastRequest.rngRequestId)) { // if the request failed
        revert AlreadyStartedDraw();

Since isRequestFailed() might wrongly return true as described above, it is possible for draw bots to retry a randomness request even when a random number is actually available.

Additionally, it is possible for startDraw() to be called after a draw has concluded with finishDraw(). For example:

• startDraw() is called at block.number = 100.
• Witnet receives a randomness request from another user/protocol at block.number = 101.
• The randomness request at block.number = 100 fails.
• The randomness request at block.number = 101 succeeds.
• finishDraw() is called, which works as there is a successful request after block.number = 100.
• Now, if startDraw() is called, it will pass as isRequestFailed() returns true for the request at block.number = 100.

Note that it is not feasible for draw bots to check if finishDraw() has been called before calling startDraw(), as another draw bot can front-run their transaction calling startDraw() to call finishDraw().

Impact

startDraw() can be called to retry the current draw even when (a) a random number is already available, or (b) when the current draw has already finished. (a) causes a loss of funds to other draw bots that called startDraw() in the current draw as their rewards are diluted, while (b) causes a loss of funds to the draw bots that call startDraw() after the draw has finished as they never receive rewards.

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-rng-witnet/src/RngWitnet.sol#L115-L118

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-draw-manager/src/DrawManager.sol#L238-L239

Tool used

Manual Review

Recommendation

In startDraw(), consider checking isRequestComplete() to know if a random number is available:

-     if (!rng.isRequestFailed(lastRequest.rngRequestId)) { // if the request failed
+     if (rng.isRequestComplete(lastRequest.rngRequestId)) { // if the request failed
        revert AlreadyStartedDraw();

[10xhash]

Escalate

Here we are speculating on the behavior of the bots which I believe would be out of scope. The bot would have to call the startDraw function even when the draw has finished or in an edge case (later rng request being reported earlier and earlier being reported ERROR) the bot would have the option to either call startDraw or call finishDraw

The recommendation is incorrect because it would allow all new requests to be overwritten. The current implementation is done with the assumption that witnet requests will be reported sequentially (which would be the case most of the time). In which case, as soon as the request results in an ERROR, the draw awarding can be reattempted. In case it doesn't happen sequentially and this request returns ERROR while the newer request returns a correct value, there is the option to either restart the draw or finish the draw and it would be depending on the bot's implementation which of these is performed

[sherlock-admin3]

Escalate

Here we are speculating on the behavior of the bots which I believe would be out of scope. The bot would have to call the startDraw function even when the draw has finished or in an edge case (later rng request being reported earlier and earlier being reported ERROR) the bot would have the option to either call startDraw or call finishDraw

The recommendation is incorrect because it would allow all new requests to be overwritten. The current implementation is done with the assumption that witnet requests will be reported sequentially (which would be the case most of the time). In which case, as soon as the request results in an ERROR, the draw awarding can be reattempted. In case it doesn't happen sequentially and this request returns ERROR while the newer request returns a correct value, there is the option to either restart the draw or finish the draw and it would be depending on the bot's implementation which of these is performed

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[MiloTruck]

In case it doesn't happen sequentially and this request returns ERROR while the newer request returns a correct value, there is the option to either restart the draw or finish the draw and it would be depending on the bot's implementation which of these is performed

The current implementation allows for the following to occur:

• Bot calls startDraw() with the first request.
• First request returns ERROR.
• Second request returns a correct value.
• Bot calls startDraw() to restart the draw.
• Attacker front-runs the bot to call finishDraw().
• Bot's call to startDraw() still executes and passes, causing it to have called startDraw() after finishDraw().

There is no requirement for a bot to act abnormally here, it chooses the restart the draw as you have stated and ends up losing funds.

Even if the impacts/scenarios listed in my issue can only occur under an edge case or with bots behaving in a certain manner, I don't believe it is an acceptable risk for bots to potentially lose funds just by interacting with the protocol.

[WangSecurity]

To confirm:

For example, assume there are two requests:

Request 1 made at block.number = 100 failed. Request 2 made at block.number = 101 was successful. If isRandomized() and fetchRandomnessAfter() was called for block number 100, they would return true and a valid random number respectively. By extension, RngWitnet.isRequestComplete() would return true for request 1. However, RngWitnet.isRequestFailed() also returns true for request 1, forming a contradiction.

Even if the request failed, isRandomized, fetchRandomnessAfter and RngWitnet.isRequestComplete would return true as if the request didn't fail. And RngWitnet.isRequestFailed would also return true, because it's not correctly implemented (the root cause), could you please clarify this part briefly again, I feel like I'm missing something?

[MiloTruck]

@WangSecurity I'm not really sure what you don't understand.

isRandomized() checks if there is a successful request at a specific block or any block after it, while isRequestFailed() only checks if the request at a specific block failed. So in this scenario isRandomized() returns true for block 100 since a block after it contains a successful request (ie. request 2 at block 101), while isRequestFailed() returns false as request 1 at block 100 failed.

[WangSecurity]

Thank you for clarification, based on this and that, I believe it doesn't require for the bots to can in any strange way and it doesn't require any mistake on their end. Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 10xhash: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/GenerationSoftware/pt-v5-rng-witnet/pull/8

[10xhash]

Fixed Now the exact request has to be solved for the query to be considered complete (instead of isRandomized returning true) disallowing a request to exist as both failed and complete at the same time

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.