code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L24 https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L165

Vulnerability details

Impact

The admin can not set the minimumRequestConfirmations value upon initializating VRFNFTRandomDraw as it is fixed to the default value 3.

    /// @notice Chainlink request confirmations, left at the default
    uint16 immutable minimumRequestConfirmations = 3;

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L24

This gives the admin no control over defining an important security parameter. The greater the value is, the more secure the randomness is (protection from block re-organizations) and the higher the latency is (time between request and response). I believe this balance between security and performance should be set/decided by the admin.

According to ChainLink's VRF security consideration:

In principle, miners/validators of your underlying blockchain could rewrite the chain’s history to put a randomness request from your contract into a different block, which would result in a different VRF output. Note that this does not enable a miner to determine the random value in advance. It only enables them to get a fresh random value that might or might not be to their advantage. By way of analogy, they can only re-roll the dice, not predetermine or predict which side it will land on.

https://docs.chain.link/vrf/v2/security#choose-a-safe-block-confirmation-time-which-will-vary-between-blockchains

I've submitted this issue since it's configuration relaated, and according to the contest docs:

Any issues directly related to the chainlink infastructure contracts other than incorrect configuration of their libraries within this project are not in scope for this audit.

Proof of Concept

Upon requesting a random number the default value is used as follows:

 // Request first random round
        request.currentChainlinkRequestId = coordinator.requestRandomWords({
            keyHash: settings.keyHash,
            subId: settings.subscriptionId,
            minimumRequestConfirmations: minimumRequestConfirmations,
            callbackGasLimit: callbackGasLimit,
            numWords: wordsRequested
        });

https://github.com/code-423n4/2022-12-forgeries/blob/main/src/VRFNFTRandomDraw.sol#L165

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider adding a new parameter request confirmations number in the settings struct. https://github.com/code-423n4/2022-12-forgeries/blob/main/src/interfaces/IVRFNFTRandomDraw.sol#L71-L90
Upon initializating VRFNFTRandomDraw, check if the number within the allowed range (minimum 3 - maximum 200), otherwise, revert.

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)