TangibleTNFT / baskets-foundry

baskets-foundry
1 stars 0 forks source link

[GVR-01M] Non-Compliance w/ Checks-Effects-Interactions Pattern #30

Closed chasebrownn closed 7 months ago

chasebrownn commented 7 months ago

GVR-01M: Non-Compliance w/ Checks-Effects-Interactions Pattern

Type Severity Location
Language Specific GelatoVRFConsumerBase.sol:L95, L96, L133, L134

Description:

The GelatoVRFConsumerBase::fulfillRandomnessTestnet and GelatoVRFConsumerBase::fulfillRandomness functions will fulfil randomness via the GelatoVRFConsumerBase::_fulfillRandomness function before setting the requestId pending status to false, permitting a theoretical re-entrancy attack to be executed.

Example:

/// @notice Callback function used by Gelato VRF to return the random number.
/// The randomness is derived by hashing the provided randomness with the request ID.
/// @param randomness The random number generated by Gelato VRF.
/// @param dataWithRound Additional data provided by Gelato VRF containing request details.
function fulfillRandomness(
    uint256 randomness,
    bytes calldata dataWithRound
) external {
    require(msg.sender == _operator(), "only operator");

    (, bytes memory data) = abi.decode(dataWithRound, (uint256, bytes));
    (uint256 requestId, bytes memory extraData) = abi.decode(
        data,
        (uint256, bytes)
    );

    bytes32 requestHash = keccak256(dataWithRound);
    bool isValidRequestHash = requestHash == requestedHash[requestId];

    require(requestPending[requestId], "request fulfilled or missing");

    if (isValidRequestHash) {
        randomness = uint(
            keccak256(
                abi.encode(
                    randomness,
                    address(this),
                    block.chainid,
                    requestId
                )
            )
        );

        _fulfillRandomness(randomness, requestId, extraData);
        requestPending[requestId] = false;
    }
}

Recommendation:

While the BasketsVrfConsumer::_fulfillRandomness function which in turn invokes Basket::fulfillRandomSeed does not expose a re-entrant call, it is still advisable for the code to adhere to the CEI pattern and set a request's pending status to false prior to fulfilling it.

chasebrownn commented 7 months ago

Fixed