sherlock-audit / 2024-01-looksrare-judging

3 stars 0 forks source link

ast3ros - Inadequate minimum request confirmations #142

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

ast3ros

medium

Inadequate minimum request confirmations

Summary

The minimumRequestConfirmations parameter is currently set to a fixed value of 3 in the smart contract, which may not be suitable for other L2 chains

Vulnerability Detail

The contracts are deployed to ETH mainnet and L2s such as Arbitrum and potentially any EVM compatible L2

When sending a request for random words, the _drawWinner function sets the minimumRequestConfirmations to 3.

    uint256 requestId = VRF_COORDINATOR.requestRandomWords({
        keyHash: KEY_HASH,
        subId: SUBSCRIPTION_ID,
        minimumRequestConfirmations: uint16(3),
        callbackGasLimit: uint32(500_000),
        numWords: uint32(1)
    });

https://github.com/sherlock-audit/2024-01-looksrare/blob/314fd5b753359e07778724342de8dda20713ec2e/contracts-yolo/contracts/YoloV2.sol#L1001-L1007

However, Chainlink's documentation on security considerations advises tailoring the minimum request confirmation count to each specific blockchain:

    You must choose an appropriate confirmation time for the randomness requests you make. Confirmation time is how many blocks the VRF service waits before writing a fulfillment to the chain to make potential rewrite attacks unprofitable in the context of your application and its value-at-risk.

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

In practice, as observed in centralized exchange (CEX) implementations for L2 deposits, a significant number of block confirmations are awaited (e.g., minimum 10 ETH blocks for Arbitrum and 12 for Optimism). This suggests that a higher minimum request confirmation than 3 could enhance security.

https://medium.com/chainlight/patch-thursday-risks-on-cexs-confirmation-on-arbitrum-and-optimism-7ee25a1d58bf

Furthermore, VRFv2 supports up to 200 block confirmations, providing adequate range for adjustments.

https://docs.chain.link/vrf/v2/subscription/supported-networks#arbitrum-mainnet

Impact

With the current low minimumRequestConfirmations, there's an increased risk of redrawing the random word, potentially compromising the fairness of outcomes. For example, a winner determined in the first draw might lose their rewards due to a subsequent redraw.

Code Snippet

https://github.com/sherlock-audit/2024-01-looksrare/blob/314fd5b753359e07778724342de8dda20713ec2e/contracts-yolo/contracts/YoloV2.sol#L1001-L1007

Tool used

Manual review

Recommendation

Adjust the minimumRequestConfirmations to a dynamic parameter and set during contract deployment for each network.

Duplicate of #41