From 5d951ee755a9b58c1434d9a79b305bc3e862eff1 Mon Sep 17 00:00:00 2001
From: Dev Kalra <kalradev@outlook.com>
Date: Thu, 14 Dec 2023 17:57:45 +0530
Subject: [PATCH] [entropy] audit 4. users can influence the Entropy revealed
 result (#1179)

* add a check for blockhash

* add comment

* update test name

* comment update

* change block number

* change order for assertions

* pre commit run

* names and comments
---
 .../contracts/contracts/entropy/Entropy.sol   | 13 +++-
 .../contracts/forge-test/Entropy.t.sol        | 75 +++++++++++++++++++
 .../forge-test/EntropyGasBenchmark.t.sol      |  3 +-
 .../entropy_sdk/solidity/EntropyErrors.sol    |  2 +
 4 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol
index d8befcec..40ab41e2 100644
--- a/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol
+++ b/target_chains/ethereum/contracts/contracts/entropy/Entropy.sol
@@ -259,7 +259,18 @@ abstract contract Entropy is IEntropy, EntropyState {
 
         bytes32 blockHash = bytes32(uint256(0));
         if (req.useBlockhash) {
-            blockHash = blockhash(req.blockNumber);
+            bytes32 _blockHash = blockhash(req.blockNumber);
+
+            // The `blockhash` function will return zero if the req.blockNumber is equal to the current
+            // block number, or if it is not within the 256 most recent blocks. This allows the user to
+            // select between two random numbers by executing the reveal function in the same block as the
+            // request, or after 256 blocks. This gives each user two chances to get a favorable result on
+            // each request.
+            // Revert this transaction for when the blockHash is 0;
+            if (_blockHash == bytes32(uint256(0)))
+                revert EntropyErrors.BlockhashUnavailable();
+
+            blockHash = _blockHash;
         }
 
         randomNumber = combineRandomValues(
diff --git a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol
index 7d26dc18..38b79558 100644
--- a/target_chains/ethereum/contracts/forge-test/Entropy.t.sol
+++ b/target_chains/ethereum/contracts/forge-test/Entropy.t.sol
@@ -405,6 +405,81 @@ contract EntropyTest is Test, EntropyTestUtils {
             true
         );
 
+        vm.roll(1235);
+        assertRevealSucceeds(
+            user2,
+            provider1,
+            sequenceNumber,
+            42,
+            provider1Proofs[sequenceNumber],
+            blockhash(1234)
+        );
+    }
+
+    function testNoCheckOnBlockNumberWhenNoBlockHashUsed() public {
+        vm.roll(1234);
+        uint64 sequenceNumber = request(user2, provider1, 42, false);
+
+        vm.roll(1236);
+        assertRevealSucceeds(
+            user2,
+            provider1,
+            sequenceNumber,
+            42,
+            provider1Proofs[sequenceNumber],
+            ALL_ZEROS
+        );
+
+        vm.roll(1234);
+        sequenceNumber = request(user2, provider1, 42, false);
+
+        vm.roll(1234);
+        assertRevealSucceeds(
+            user2,
+            provider1,
+            sequenceNumber,
+            42,
+            provider1Proofs[sequenceNumber],
+            ALL_ZEROS
+        );
+
+        vm.roll(1234);
+        sequenceNumber = request(user2, provider1, 42, false);
+
+        vm.roll(1234 + 257);
+        assertRevealSucceeds(
+            user2,
+            provider1,
+            sequenceNumber,
+            42,
+            provider1Proofs[sequenceNumber],
+            ALL_ZEROS
+        );
+    }
+
+    function testCheckOnBlockNumberWhenBlockHashUsed() public {
+        vm.roll(1234);
+        uint64 sequenceNumber = request(user2, provider1, 42, true);
+
+        vm.roll(1234);
+        assertRevealReverts(
+            user2,
+            provider1,
+            sequenceNumber,
+            42,
+            provider1Proofs[sequenceNumber]
+        );
+
+        vm.roll(1234 + 257);
+        assertRevealReverts(
+            user2,
+            provider1,
+            sequenceNumber,
+            42,
+            provider1Proofs[sequenceNumber]
+        );
+
+        vm.roll(1235);
         assertRevealSucceeds(
             user2,
             provider1,
diff --git a/target_chains/ethereum/contracts/forge-test/EntropyGasBenchmark.t.sol b/target_chains/ethereum/contracts/forge-test/EntropyGasBenchmark.t.sol
index 86216b0a..32388fb4 100644
--- a/target_chains/ethereum/contracts/forge-test/EntropyGasBenchmark.t.sol
+++ b/target_chains/ethereum/contracts/forge-test/EntropyGasBenchmark.t.sol
@@ -107,8 +107,9 @@ contract EntropyGasBenchmark is Test, EntropyTestUtils {
 
     function testBasicFlow() public {
         uint userRandom = 42;
+        vm.roll(10);
         uint64 sequenceNumber = requestHelper(user1, userRandom, true);
-
+        vm.roll(12);
         revealHelper(sequenceNumber, userRandom);
     }
 }
diff --git a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol
index a13fe770..18deee8d 100644
--- a/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol
+++ b/target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol
@@ -22,4 +22,6 @@ library EntropyErrors {
     error InvalidUpgradeMagic();
     // The msg.sender is not allowed to invoke this call.
     error Unauthorized();
+    // The blockhash is 0.
+    error BlockhashUnavailable();
 }