Raffle is fair only if `tokenRange` is a power of 2.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-forgeries/blob/fc271cf20c05ce857d967728edfb368c58881d85/src/VRFNFTRandomDraw.sol#L254-L256

Vulnerability details

Impact

Based on the contract logic, the raffle works by sampling a value $ {x \in \mathbb{Z} | 0 \leq x \lt 2^{256} } $ (aka uint256) from a uniform distribution (provided by Chainlink VRF), then applying modulo tokenRange, generating a new value $ {y \in \mathbb{Z} | 0 \leq y \lt tokenRange } $. Consider Y the probability mass function of $ y $, then this raffle can be considered fair only if all the values $y$ in tokenRange have the same probability of being chosen ($ Pr(Y = y) = \frac{1}{tokenRange} $). But this is only true if tokenRange is a power of 2. If tokenRange is not a power of 2, then not all possible values of $y$ have the same probability and it can be shown that the relative difference in probability between the value $y$ with the highest probability and the value $y$ with the lowest probability is $ \approx \frac{tokenRange}{2^{256}} $. Therefore for most tokenRange this can be considered negligible, although still theoretically an unfair raffle.

The ERC721 Standart does not set a upper limit on the number of tokens, so in some edge cases where tokenRange can be chosen to be arbitrarily large this difference in probabilities can become a real issue.

Recommended Mitigation Steps

Please consider checking if tokenRange is a power of 2. So the raffle can be considered fair no matter how large tokenRange is.

diff --git a/VRFNFTRandomDraw.sol.orig b/VRFNFTRandomDraw.sol
index 668bc56..333a0ff 100644
--- a/VRFNFTRandomDraw.sol.orig
+++ b/VRFNFTRandomDraw.sol
@@ -111,7 +111,8 @@ contract VRFNFTRandomDraw is
         // and the size of the range needs to be at least 2 (end is exclusive)
         if (
             _settings.drawingTokenEndId < _settings.drawingTokenStartId ||
-            _settings.drawingTokenEndId - _settings.drawingTokenStartId < 2
+            _settings.drawingTokenEndId - _settings.drawingTokenStartId < 2 ||
+            (_settings.drawingTokenEndId - _settings.drawingTokenStartId) % 2 == 0
         ) {
             revert DRAWING_TOKEN_RANGE_INVALID();
         }

And also modify the tests to conform to this new check.

diff --git a/VRFNFTRandomDraw.t.sol.orig b/VRFNFTRandomDraw.t.sol
index a023626..0d2c9fa 100644
--- a/VRFNFTRandomDraw.t.sol.orig
+++ b/VRFNFTRandomDraw.t.sol
@@ -65,7 +65,7 @@ address sender = address(0x994);
         settings.token = address(targetNFT);
         settings.tokenId = 0;
         settings.drawingTokenStartId = 0;
-        settings.drawingTokenEndId = 2;
+        settings.drawingTokenEndId = 3;
         settings.drawingToken = address(drawingNFT);
         settings.subscriptionId = subscriptionId;

@@ -138,7 +138,7 @@ address sender = address(0x994);
         settings.recoverTimelock = 2 weeks;
         settings.token = address(targetNFT);
         settings.drawingTokenStartId = 0;
-        settings.drawingTokenEndId = 4;
+        settings.drawingTokenEndId = 3;
         settings.drawingToken = address(drawingNFT);

         // recovery timelock too soon
@@ -171,7 +171,7 @@ address sender = address(0x994);
         settings.token = address(targetNFT);
         settings.tokenId = 0;
         settings.drawingTokenStartId = 0;
-        settings.drawingTokenEndId = 2;
+        settings.drawingTokenEndId = 3;
         settings.drawingToken = address(drawingNFT);
         settings.subscriptionId = subscriptionId;

@@ -211,7 +211,7 @@ address sender = address(0x994);
                 tokenId: 0,
                 drawingToken: address(drawingNFT),
                 drawingTokenStartId: 0,
-                drawingTokenEndId: 10,
+                drawingTokenEndId: 7,
                 drawBufferTime: 1 hours,
                 recoverTimelock: 2 weeks,
                 keyHash: bytes32(
@@ -258,7 +258,7 @@ address sender = address(0x994);
                 tokenId: 0,
                 drawingToken: address(drawingNFT),
                 drawingTokenStartId: 0,
-                drawingTokenEndId: 10,
+                drawingTokenEndId: 7,
                 drawBufferTime: 1 hours,
                 recoverTimelock: 2 weeks,
                 keyHash: bytes32(
@@ -320,7 +320,7 @@ address sender = address(0x994);
                 tokenId: 0,
                 drawingToken: address(drawingNFT),
                 drawingTokenStartId: 0,
-                drawingTokenEndId: 10,
+                drawingTokenEndId: 7,
                 drawBufferTime: 1 hours,
                 recoverTimelock: 2 weeks,
                 keyHash: bytes32(
@@ -373,7 +373,7 @@ address sender = address(0x994);
                 tokenId: 0,
                 drawingToken: address(drawingNFT),
                 drawingTokenStartId: 0,
-                drawingTokenEndId: 10,
+                drawingTokenEndId: 7,
                 drawBufferTime: 1 hours,
                 recoverTimelock: 2 weeks,
                 keyHash: bytes32(
@@ -448,7 +448,7 @@ address sender = address(0x994);
                 tokenId: 0,
                 drawingToken: address(drawingNFT),
                 drawingTokenStartId: 0,
-                drawingTokenEndId: 10,
+                drawingTokenEndId: 7,
                 drawBufferTime: 1 hours,
                 recoverTimelock: 2 weeks,
                 keyHash: bytes32(
@@ -520,7 +520,7 @@ address sender = address(0x994);
                 tokenId: 0,
                 drawingToken: address(drawingNFT),
                 drawingTokenStartId: 0,
-                drawingTokenEndId: 10,
+                drawingTokenEndId: 7,
                 drawBufferTime: 1 hours,
                 recoverTimelock: 2 weeks,
                 keyHash: bytes32(

[gzeoneth]

realistically this is not an issue give the gas limit

[c4-judge]

gzeon-c4 marked the issue as unsatisfactory: Invalid