Forgeries contest - codeislight's results

Lines of code

https://github.com/smartcontractkit/chainlink/blob/e1e78865d4f3e609e7977777d7fb0604913b63ed/contracts/src/v0.8/VRFCoordinatorV2.sol#L407

Vulnerability details

Impact

• VRFNFTRandomDraw.initialize() must check keyHash to be one of the corresponding keyHashes for GasPrice supported by the coordinator before assigning the settings storage variable.
• Note that Chainlink coordinator doesn't check the validity of keyHashes to save on gas fees on their end, refer to the Github link down below.

Proof of Concept

• when the admin enters VRFNFTRandomDrawFactory.makeNewDraw() an invalid settings.keyHash which is being assigned in VRFNFTRandomDraw.initialize() function, once a draw or redraw takes place, the randomness will never get fulfilled.
• thus, it would impact the new deployed VRFNFTRandomDraw core functionality, and would halt draw/redraw raffles. which would force the admin to wait for the recovery timestamp to be reached to be able to recover the NFT that ranges between a week up to a year. https://vrf.chain.link/mainnet https://github.com/smartcontractkit/chainlink/blob/e1e78865d4f3e609e7977777d7fb0604913b63ed/contracts/src/v0.8/VRFCoordinatorV2.sol#L407

Tools Used

no tools were used, just manual analysis

Recommended Mitigation Steps

a practical mitigation would be to have a mapping to check the keyHash validity in VRFNFTRandomDrawFactory:

    mapping (bytes32 => bool) public isValidKeyHash;
    /// @notice Constructor to set the implementation
    constructor(address _implementation, bytes32[] calldata _keyHashes) initializer {
        ...
        for(uint i = 0; i < _keyHashes.length;){
            isValidKeyHash[_keyHashes[i]] = true;
            unchecked {
                ++i;
            }
        }
    }
    ...
    error InvalidKeyHash();
    /// @notice Function to make a new drawing
    /// @param settings settings for the new drawing
    function makeNewDraw(IVRFNFTRandomDraw.Settings memory settings)
        external
        returns (address)
    {
        if(!isValidKeyHash[settings.keyHash]) revert InvalidKeyHash();
        ...
    }

• in makeNewDraw function, newDrawing variable could be defined as a variable in the return area to save some gas

    function makeNewDraw(IVRFNFTRandomDraw.Settings memory settings)
        external
        returns (address newDrawing)
    {
        address admin = msg.sender;
        // Clone the contract
        newDrawing = ClonesUpgradeable.clone(implementation);
        // Setup the new drawing
        IVRFNFTRandomDraw(newDrawing).initialize(admin, settings);
        // Emit event for indexing
        emit SetupNewDrawing(admin, newDrawing);
    }
  

• use unchecked in calculations that is impossible to overflow:

    function _requestRoll() internal {
       ...
        unchecked{
            // Setup re-draw timelock
            request.drawTimelock = block.timestamp + settings.drawBufferTime;
        }
        ...
    }
  
    function fulfillRandomWords(
        uint256 _requestId,
        uint256[] memory _randomWords
    ) internal override {
        ...
        unchecked {
            // Get total token range
            uint256 tokenRange = settings.drawingTokenEndId -
                settings.drawingTokenStartId;
            // Store a number from it here (reduce number here to reduce gas usage)
            // We know there will only be 1 word sent at this point.
            request.currentChosenTokenId =
                (_randomWords[0] % tokenRange) +
                settings.drawingTokenStartId;
        }
        ...
    }
  

• assign state variables after revert checks, to save gas on SSTORE in case it reverts:

    function initialize(address admin, Settings memory _settings)
        public
        initializer
    {
        // Check values in memory:
        if (_settings.drawBufferTime < HOUR_IN_SECONDS) {
            revert REDRAW_TIMELOCK_NEEDS_TO_BE_MORE_THAN_AN_HOUR();
        }
        if (_settings.drawBufferTime > MONTH_IN_SECONDS) {
            revert REDRAW_TIMELOCK_NEEDS_TO_BE_LESS_THAN_A_MONTH();
        }
  
        ...
  
        // Set new settings
        settings = _settings;
        ...
    }
  

• emit events at the end of the functions to save gas on reverts, the following are the instances:

    emit InitializedDraw(msg.sender, settings);
  
    emit SetupDraw(msg.sender, settings);
  
    emit WinnerSentNFT(
        user,
        address(settings.token),
        settings.tokenId,
        settings
    );
  
    emit OwnerReclaimedNFT(owner());