search

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

0 stars 0 forks source link

LOWER BOUNDARY OF DRAWING TOKEN RANGE IS TOO LOW #348

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#L114

Vulnerability details

Impact

The current logic in VRFNFTRandomDraw.sol could lead to undesirable edge cases due to allowing the lower limit of the drawing token range to be as low as 2. It could lead to a long drag before the raffle could end or cancel if one of the drawing tokens is inactive.

Proof of Concept

Here is the scenario:

  1. _settings.drawBufferTime was set to 1 month
  2. _settings.recoverTimelock was set to 1 year (12 months)
  3. The selected winner had been permanently burned or inactive.
  4. After 1 month, a redraw was executed. But, there was a 50% chance step 1. would have to be repeated.

Recommended Mitigation Steps

It is recommended setting the affected code below to a higher and sensible token range:

VRFNFTRandomDraw.sol#L114 

            _settings.drawingTokenEndId - _settings.drawingTokenStartId < 2

Alternatively, implement a function logic that would first add all drawing token IDs to an array. If a redraw is needed, remove the inactive winner from the list. In that case, the randomWords returned by Chainlink will have to be modded by the latest array length which will also serve as the array index then to get an assigned value of request.currentChosenTokenId.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid

gzeoneth commented 1 year ago

They are allowed to configure a lower value.