A validator can manipulate the Chainlink request order to reroll the randomNumber in his favor

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/RNSourceController.sol#L34

Vulnerability details

There is a retry function in RNSourceController that is used to request another randomNumber to Chainlink, in case anything goes wrong with the first request called by the Lottery.

This retry can be performed in an interval defined by maxRequestDelay.

If a jackpot becomes big enough, a validator could be interested in manipulating the order request to effectively 'reroll the dice' in his favor, thus highly increasing his chance of winning the lottery.

Impact

A validator can manipulate the Chainlink request order to reroll the randomNumber in his favor, increasing his chance of winning the lottery.

Proof of Concept

There is no lower limit for maxRequestDelay:

File: src/RNSourceController.sol

26:     constructor(uint256 _maxFailedAttempts, uint256 _maxRequestDelay) {
27:         if (_maxFailedAttempts > MAX_MAX_FAILED_ATTEMPTS) {
28:             revert MaxFailedAttemptsTooBig();
29:         }
30:         if (_maxRequestDelay > MAX_REQUEST_DELAY) {
31:             revert MaxRequestDelayTooBig();
32:         }
33:         maxFailedAttempts = _maxFailedAttempts;
34:         maxRequestDelay = _maxRequestDelay;
35:     }

The average block time on Polygon is 2, meaning a new block is produced every 2 seconds.

This means that this attack can be performed if the following is true:

maxRequestDelay < ~(2 seconds)

Let's suppose that a Lottery is initialized with maxRequestDelay = 1 seconds

1. The executeDraw function is called, and Chainlink is invoked through requestRandomNumber
2. Transaction succeeds and now lastRequestFulfilled is false
3. After that, Chainlink calls receiveRandomNumber: let's call this pending transaction transaction_A
4. A user (or the validator himself) calls the retry function after 1 second, and a new requestRandomNumberFromSource is made
5. Chainlink calls receiveRandomNumber again, let's call this pending transaction transaction_B
6. A validator receives both transactions, and he can control the order in which the requests appear on-chain, so he can change the order in which the contract will respond
7. In this case he decides to put transaction_B before transaction_A, because the randomNumber generated is better for him in the former transaction
8. Lottery finalizes the draw with the randomNumber from transaction_B, and now drawExecutionInProgress is false
9. transaction_A reverts with a DrawNotInProgress error

Tools Used

Manual review

Recommended Mitigation Steps

Put a lower limit for maxRequestDelay to avoid this type of attack, to be safe it should be some minutes.

To compare, Ethereum mainnet currently takes an average of 12 seconds, while Bitcoin takes an average of 10 minutes per block.

[c4-judge]

thereksfour marked the issue as duplicate of #445

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)