0x73696d616f - VRFCoordination minimum confirmation change will DoS randomness requests leading to stuck prizes and ETH

[sherlock-admin4]

0x73696d616f

Medium

VRFCoordination minimum confirmation change will DoS randomness requests leading to stuck prizes and ETH

Summary

The block confirmations argument in WinnablesTicketManager::drawWinner() is hardcoded to 3, which may be less than the s_config.minimumRequestConfirmations in the VRF coordination. This will make WinnablesTicketManager::drawWinner() revert, so the raffle can neither be claimed nor canceled.

This may happen because the VRF coordinator has a function to change the minimum request confirmations setConfig(), line 230.

Root Cause

In WinnablesTicketManager:318, requestConfirmations is hardcoded to 3. In the VRF coordinator, line 390, it checks requestConfirmations < s_config.minimumRequestConfirmations, which may revert if s_config.minimumRequestConfirmations > 3.

Internal pre-conditions

None.

External pre-conditions

s_config.minimumRequestConfirmations is changed by Chainlink.

Attack Path

1. Enough tickets are bought in a raffle.
2. Chainlink modifies the minimum number of block confirmations to a value above 3.
3. WinnablesTicketManager::drawWinner() reverts.

Impact

Stuck ETH and prizes.

PoC

The informations above can be confirmed with the links.

Mitigation

Get the minimum number of block confirmations from the config in WinnablesTicketManager::drawWinner() and send it if smaller than 3.

[0xsimao]

Escalate

This finding should be valid as Chainlink may change the minimum confirmation blocks at any time, see line 230 here.

[sherlock-admin3]

Escalate

This finding should be valid as Chainlink may change the minimum confirmation blocks at any time, see line 230 here.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Brivan-26]

This should be invalid per contest README:

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

No

[0xsimao]

@brivan-26 the rule applies to future integrations; however, this issue is not a future integration, it's the current Chainlink code, hence valid.

[Oblivionis214]

https://docs.chain.link/vrf/v2/direct-funding/supported-networks#avalanche-mainnet

Minimum Confirmations = 1. There is no evidence this value may/should change in the future.

requestConfirmations = 3 works well for current avalanche mechanism.

[mystery0x]

Much as hardcoding isn't a good engineering practice, this finding is low at best because:

1. This is indeed a future but unlikely change by Chainlink just like its price feed addresses where many protocol has chosen to hardcode.
2. The logic is working flawlessly as of now.

[0xsimao]

@mystery0x it's not the same as hardcoding addresses because for that to be harmful new code would have to be deployed. Here the current code itself is vulnerable, so not a future integration.

[Brivan-26]

The current Minimum Confirmations configuration is set to 3, as demonstrated by the Chainlink docs. The issue is about future integration break with another protocol

[WangSecurity]

The current minimumRequestConfirmations is 3 and the protocol works without any issues. This variable is set by the Chainlink admins, therefore, the following rule applies:

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path. If no values are provided, the (external) admin is trusted to use values that will not cause any issues. Note: if the attack path is possible with any possible value, it will be a valid issue. Exception: It will be a valid issue if the attack path is based on a value currently live on the network to which the protocol will be deployed.

There are no limitations for values for the external admins, therefore, we should assume they will use values that won't cause any issues. There are values that won't cause any issues and even the current on-chain value doesn't pose an issue.

Therefore, I believe this issue should remain invalid. Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xsimao: rejected