code-423n4 / 2022-12-forgeries-findings

0 stars 0 forks source link

Function `fulfillRandomWords` in VRFNFTRandomDraw contract must not revert #322

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

The VRFNFTRandomDraw contract implements the Chainlink VFR feature to pull random data to select the raffle winner. As per their security guidelines the implementation of the fulfillRandomWords function must not revert.

Impact

If the fulfillRandomWords function reverts, then the service won't attempt to call it again, which will prevent it from pulling random data into the contract. As per the cited docs:

If your fulfillRandomWords() implementation reverts, the VRF service will not attempt to call it a second time. Make sure your contract logic does not revert.

PoC - Code Snippet

If _requestId != request.currentChainlinkRequestId or _randomWords.length != wordsRequested the fulfillRandomWords function will revert:

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

/// @notice Function called by chainlink to resolve random words
/// @param _requestId ID of request sent to chainlink VRF
/// @param _randomWords List of uint256 words of random entropy
function fulfillRandomWords(
    uint256 _requestId,
    uint256[] memory _randomWords
) internal override {
    // Validate request ID
    if (_requestId != request.currentChainlinkRequestId) {
        revert REQUEST_DOES_NOT_MATCH_CURRENT_ID();
    }

    // Validate number of words returned
    // Words requested is an immutable set to 1
    if (_randomWords.length != wordsRequested) {
        revert WRONG_LENGTH_FOR_RANDOM_WORDS();
    }

    // Set request details
    request.hasChosenRandomNumber = true;

    // 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;

    // Emit completed event.
    emit DiceRollComplete(msg.sender, request);
}

Recommendation

If _requestId or _randomWords aren't as expected, then ignore the response instead of reverting. If something goes wrong, then redraw can be called (after the draw timelock passes) to trigger a new request.

// Validate request ID
if (_requestId != request.currentChainlinkRequestId) {
    return;
}

// Validate number of words returned
// Words requested is an immutable set to 1
if (_randomWords.length != wordsRequested) {
    return;
}
trust1995 commented 1 year ago

This is desired behavior. If one of the conditions which cause revert occurs, we don't want that requestID response to be sent again

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid

romeroadrian commented 1 year ago

@trust1995 I read this as the service won't be calling again not only for that specific request. Can't find a good source to confirm this, do you have something I could check? thanks