mstpr-brainbot - Rounds can not be immediately drawn after fulfillRandomWords due to VRF contracts reentrancy guard

[sherlock-admin]

mstpr-brainbot

high

Rounds can not be immediately drawn after fulfillRandomWords due to VRF contracts reentrancy guard

Summary

Users can deposit ETH to multiple future rounds. If the future round can be "drawn" immediately when the current round ends this execution will revert because of chainlink contracts reentrancy guard protection hence ending the current round will be impossible and it needs to be cancelled by governance.

Vulnerability Detail

When the round is Drawn, VRF is expected to call fulfillRandomWords to determine the winner in the current round and starts the next round. https://github.com/sherlock-audit/2024-01-looksrare/blob/7d76b96a58a6aee38f23bb38b8a5daa3bdc03f7c/contracts-yolo/contracts/YoloV2.sol#L1293

fulFillRandomWords function is triggered by the rawFulfillRandomWords external function in the Yolo contract which is also triggered by the randomness providers call to VRF's fulFillRandomWords function which has a reentrancy check as seen and just before the call to Yolo contract it sets to "true". https://github.com/smartcontractkit/chainlink/blob/6133df8a2a8b527155a8a822d2924d5ca4bfd122/contracts/src/v0.8/vrf/VRFCoordinatorV2.sol#L526-L546

If the next round is "drawable" then the _startRound will draw the next round immediately and by doing so it will try to call VRF's requestRandomWords function which also has a nonreentrant modifier https://github.com/smartcontractkit/chainlink/blob/e4bde648582d55806ab7e0f8d4ea05a721ca120d/contracts/src/v0.8/vrf/VRFCoordinatorV2.sol#L349-L355

since the reentrancy lock was set to "true" in first call, the second calling requesting randomness will revert. Current "drawn" round will not be completed and there will be no winner because of the chainlink call will revert every time the VRF calls the yolo contract.

Impact

Already "drawn" round will not be concluded and there will be no winner. VRF's keepers will see their tx is reverted

Code Snippet

https://github.com/sherlock-audit/2024-01-looksrare/blob/7d76b96a58a6aee38f23bb38b8a5daa3bdc03f7c/contracts-yolo/contracts/YoloV2.sol#L949-L991

https://github.com/sherlock-audit/2024-01-looksrare/blob/7d76b96a58a6aee38f23bb38b8a5daa3bdc03f7c/contracts-yolo/contracts/YoloV2.sol#L1270-L1296

https://github.com/smartcontractkit/chainlink/blob/e4bde648582d55806ab7e0f8d4ea05a721ca120d/contracts/src/v0.8/vrf/VRFCoordinatorV2.sol#L349-L408

https://github.com/smartcontractkit/chainlink/blob/e4bde648582d55806ab7e0f8d4ea05a721ca120d/contracts/src/v0.8/vrf/VRFCoordinatorV2.sol#L526-L572

Tool used

Manual Review

Recommendation

Do not draw the contract immediately if it's winnable. Anyone can call drawWinner when the conditions are satisfied. This will slow the game pace, if that's not ideal then add an another if check to the drawWinner to conclude a round if its drawable immediately .

[0xhiroshi]

https://github.com/LooksRare/contracts-yolo/pull/175

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid: should provide POC

[nevillehuang]

request poc

[sherlock-admin]

PoC requested from @mstpr

Requests remaining: 6

[0xhiroshi]

@nevillehuang There's no need for a PoC for this issue. A medium finding makes sense to me.

[mstpr]

@nevillehuang I tried to make a PoC but couldn't mock the actual VRF call from chainlink

@0xhiroshi I believe this can be a high issue because if the fulFillRandomWords reverts chainlink random providers will not try to call the contract address. When I talked with the Chainlink team they said if the issue is fixable then fix it, if not redeploy the contract. Fixing this issue is not possible without re-writing that specific piece of the code so a redeployment is a must with the modified code.

[0xhiroshi]

While it's a non-trivial bug, technically no fund is lost, and the stuck round can be cancelled. Based on Sherlock's guidelines should this be a High? I will leave the final judgment to @nevillehuang.

[ArnieSec]

Escalate

Would like to respectfully escalate this down to invalid.

As @0xhiroshi highlights, there is no loss of funds, instead the only effect of this bug is a DOS of 24 hours, after the 24 hours, the round can be canceled.

According to sherlock documentation, if a DOS does not last more than 1 week, it does not constitute a valid issue.

The issue causes locking of funds for users for more than a week

Additionally, given that the dos is caused because of an external contract this further supports my reasoning as to why this issue is invalid. Given that the readme states that

In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.

Yes

In conclusion based on sherlock docs of what makes a valid issue, and the readme, this issue should be invalid.

[sherlock-admin2]

Escalate

Would like to respectfully escalate this down to invalid.

As @0xhiroshi highlights, there is no loss of funds, instead the only effect of this bug is a DOS of 24 hours, after the 24 hours, the round can be canceled.

According to sherlock documentation, if a DOS does not last more than 1 week, it does not constitute a valid issue.

The issue causes locking of funds for users for more than a week

Additionally, given that the dos is caused because of an external contract this further supports my reasoning as to why this issue is invalid. Given that the readme states that

In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.

Yes

In conclusion based on sherlock docs of what makes a valid issue, and the readme, this issue should be invalid.

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.

[mstpr]

LooksRare/contracts-yolo#175

Fix LGTM!

[nevillehuang]

@mstpr @ArnieGod This looks to me to be not just a regular DoS., but could occur every single time the following scenario described happens:

If the future round can be "drawn" immediately when the current round ends this execution will revert because of chainlink contracts reentrancy

So if every single time admin needs to be forced to cancel rounds, this is broken functionality to me and should constitute medium severity, since it breaks the intended game mechanism catering to the above scenario every time.

[0xhiroshi]

@mstpr This looks to me to be not just a regular DoS., but could occur every single time the following scenario described happens:

If the future round can be "drawn" immediately when the current round ends this execution will revert because of chainlink contracts reentrancy

So if every single time admin needs to be forced to cancel rounds, this is broken functionality to me and should constitute medium severity, since it breaks the intended game mechanism catering to the above scenario every time.

Yes this can happen every single time and it's broken functionality.

[mstpr]

@nevillehuang @0xhiroshi From what I see if fullFillRandomWords ever fails in chainlink the chainlink keepers will not be calling the contract once again. If that's true, the contract has to be redeployed and basically the game is fully not playable for any round. Would that make it a high issue ?

[nevillehuang]

@mstpr I don't think so, because the round can be cancelled and the deposited funds can be retrieved, than the fix can be implemented. So this is essentially a permanent DoS for that scenario.

[Czar102]

Breaking core functionality without loss of funds is a Medium severity issue.

[A medium severity issue] breaks core contract functionality, rendering the contract useless (...)

Planning to reject the escalation and leave the issue as is.

[detectiveking123]

@Czar102 Would appreciate if you could hold off on resolving this one for the next day or so. Would like to think about it for a bit.

[Czar102]

Result: Medium Unique

Sorry @detectiveking123, but I can't hold off the resolution any longer.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ArnieGod: rejected

[jacksanford1]

Sherlock note: Fix PR: https://github.com/LooksRare/contracts-yolo/pull/175

Fix has been signed off on by mstpr-brainbot