No security checks in VRF might lead to compromised results

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/VRFv2RNSource.sol#L32-L38
https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/RNSourceBase.sol#L17-L30

Vulnerability details

Impact

External actors such as Blockchain miners/validators could influence the randomness results.

Proof of Concept

This is part of the chainlink docs --> https://docs.chain.link/vrf/v2/security as they own against such an attack.

1. Inside the code, the requestRandomNumber()[https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/RNSourceBase.sol#L17-L30] function could be called as many times as possible which is against chainkLink security practices as for example, the requests A,B,C will not necessary come in the order the have been called, the contract must validate them based on their request id.

2. Also in contradictory with chainLink security standards, there is no safe block confirmation time --> https://docs.chain.link/vrf/v2/security#choose-a-safe-block-confirmation-time-which-will-vary-between-blockchains , as this could allow to miners/validators of the underlying blockchain to perform an attack where they would rewrite the chain’s history as they please to facilitate their winnings

Tools Used

Manual review, Chainlink docs

Recommended Mitigation Steps

Possible mitigation steps:

1. Enforce a minimum cooldown 24 hours for a failed response as per chainlink docs
2. Add a block confirmations time check
3. Validate based on request id's all request in the order they have been called

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)

[c4-judge]

thereksfour marked the issue as primary issue

[thereksfour]

Implementation does not meet chainlink documentation requirements

[TutaRicky]

1. requestRandomNumber can be called multiple times - no, it can't. randomness is requested by the lottery contract here - https://github.com/code-423n4/2023-03-wenwin/blob/91b89482aaedf8b8feb73c771d11c257eed997e8/src/Lottery.sol#L133, and it uses the whenNotExecutingDraw modifier which checks that randomness was not already requested.
   Other randomness requests can only be done via the retry function or swapSource function which also enforce that they can only be called in the right terms.
2. in contradictory with chainLink security standards, there is no safe block confirmation time - no. We have the requestConfirmations immutable param which is set on contract deployment

[c4-sponsor]

TutaRicky marked the issue as sponsor disputed

[c4-judge]

thereksfour marked the issue as unsatisfactory:
Invalid

[c4-judge]

thereksfour changed the severity to QA (Quality Assurance)

[c4-judge]

thereksfour marked the issue as grade-c