add authorization for reveal method (#1156)

Author: jayantk

target_chains/ethereum/contracts/contracts/entropy/Entropy.sol

@@ -206,6 +206,7 @@ abstract contract Entropy is IEntropy, EntropyState {
         req.commitment = keccak256(
             bytes.concat(userCommitment, providerInfo.currentCommitment)
         );
+        req.requester = msg.sender;
 
         if (useBlockHash) {
             req.blockNumber = SafeCast.toUint96(block.number);
@@ -223,13 +224,15 @@ abstract contract Entropy is IEntropy, EntropyState {
     // Note that this function can only be called once per in-flight request. Calling this function deletes the stored
     // request information (so that the contract doesn't use a linear amount of storage in the number of requests).
     // If you need to use the returned random number more than once, you are responsible for storing it.
+    //
+    // This function must be called by the same `msg.sender` that originally requested the random number. This check
+    // prevents denial-of-service attacks where another actor front-runs the requester's reveal transaction.
     function reveal(
         address provider,
         uint64 sequenceNumber,
         bytes32 userRandomness,
         bytes32 providerRevelation
     ) public override returns (bytes32 randomNumber) {
-        // TODO: this method may need to be authenticated to prevent griefing
         EntropyStructs.Request storage req = findRequest(
             provider,
             sequenceNumber
@@ -238,6 +241,8 @@ abstract contract Entropy is IEntropy, EntropyState {
         if (req.provider != provider || req.sequenceNumber != sequenceNumber)
             revert EntropyErrors.NoSuchRequest();
 
+        if (req.requester != msg.sender) revert EntropyErrors.Unauthorized();
+
         bytes32 providerCommitment = constructProviderCommitment(
             req.numHashes,
             providerRevelation

target_chains/ethereum/contracts/forge-test/Entropy.t.sol

@@ -99,12 +99,13 @@ contract EntropyTest is Test, EntropyTestUtils {
         bool useBlockhash
     ) public returns (uint64 sequenceNumber) {
         vm.deal(user, fee);
-        vm.prank(user);
+        vm.startPrank(user);
         sequenceNumber = random.request{value: fee}(
             provider,
             random.constructUserCommitment(bytes32(randomNumber)),
             useBlockhash
         );
+        vm.stopPrank();
     }
 
     function assertRequestReverts(
@@ -133,12 +134,14 @@ contract EntropyTest is Test, EntropyTestUtils {
     }
 
     function assertRevealSucceeds(
+        address user,
         address provider,
         uint64 sequenceNumber,
         uint userRandom,
         bytes32 providerRevelation,
         bytes32 hash
     ) public {
+        vm.prank(user);
         bytes32 randomNumber = random.reveal(
             provider,
             sequenceNumber,
@@ -156,18 +159,21 @@ contract EntropyTest is Test, EntropyTestUtils {
     }
 
     function assertRevealReverts(
+        address user,
         address provider,
         uint64 sequenceNumber,
         uint userRandom,
         bytes32 providerRevelation
     ) public {
+        vm.startPrank(user);
         vm.expectRevert();
         random.reveal(
             provider,
             sequenceNumber,
             bytes32(uint256(userRandom)),
             providerRevelation
         );
+        vm.stopPrank();
     }
 
     function assertInvariants() public {
@@ -203,6 +209,7 @@ contract EntropyTest is Test, EntropyTestUtils {
         assertEq(random.getRequest(provider1, sequenceNumber).blockNumber, 0);
 
         assertRevealSucceeds(
+            user2,
             provider1,
             sequenceNumber,
             42,
@@ -213,6 +220,7 @@ contract EntropyTest is Test, EntropyTestUtils {
         // You can only reveal the random number once. This isn't a feature of the contract per se, but it is
         // the expected behavior.
         assertRevealReverts(
+            user2,
             provider1,
             sequenceNumber,
             42,
@@ -230,13 +238,15 @@ contract EntropyTest is Test, EntropyTestUtils {
         assertEq(random.getRequest(provider1, sequenceNumber).blockNumber, 0);
 
         assertRevealReverts(
+            user2,
             random.getDefaultProvider(),
             sequenceNumber,
             42,
             provider2Proofs[sequenceNumber]
         );
 
         assertRevealSucceeds(
+            user2,
             random.getDefaultProvider(),
             sequenceNumber,
             42,
@@ -249,13 +259,37 @@ contract EntropyTest is Test, EntropyTestUtils {
         assertRequestReverts(10000000, unregisteredProvider, 42, false);
     }
 
+    function testAuthorization() public {
+        uint64 sequenceNumber = request(user2, provider1, 42, false);
+        assertEq(random.getRequest(provider1, sequenceNumber).requester, user2);
+
+        // user1 not authorized, must be user2.
+        assertRevealReverts(
+            user1,
+            provider1,
+            sequenceNumber,
+            42,
+            provider1Proofs[sequenceNumber]
+        );
+
+        assertRevealSucceeds(
+            user2,
+            provider1,
+            sequenceNumber,
+            42,
+            provider1Proofs[sequenceNumber],
+            ALL_ZEROS
+        );
+    }
+
     function testAdversarialReveal() public {
         uint64 sequenceNumber = request(user2, provider1, 42, false);
 
         // test revealing with the wrong hashes in the same chain
         for (uint256 i = 0; i < 10; i++) {
             if (i != sequenceNumber) {
                 assertRevealReverts(
+                    user2,
                     provider1,
                     sequenceNumber,
                     42,
@@ -267,6 +301,7 @@ contract EntropyTest is Test, EntropyTestUtils {
         // test revealing with the wrong user revealed value.
         for (uint256 i = 0; i < 42; i++) {
             assertRevealReverts(
+                user2,
                 provider1,
                 sequenceNumber,
                 i,
@@ -277,13 +312,14 @@ contract EntropyTest is Test, EntropyTestUtils {
         // test revealing sequence numbers that haven't been requested yet.
         for (uint64 i = sequenceNumber + 1; i < sequenceNumber + 3; i++) {
             assertRevealReverts(
+                user2,
                 provider1,
                 i,
                 42,
                 provider1Proofs[sequenceNumber]
             );
 
-            assertRevealReverts(provider1, i, 42, provider1Proofs[i]);
+            assertRevealReverts(user2, provider1, i, 42, provider1Proofs[i]);
         }
     }
 
@@ -293,21 +329,56 @@ contract EntropyTest is Test, EntropyTestUtils {
         uint64 s3 = request(user1, provider1, 3, false);
         uint64 s4 = request(user1, provider1, 4, false);
 
-        assertRevealSucceeds(provider1, s3, 3, provider1Proofs[s3], ALL_ZEROS);
+        assertRevealSucceeds(
+            user1,
+            provider1,
+            s3,
+            3,
+            provider1Proofs[s3],
+            ALL_ZEROS
+        );
         assertInvariants();
 
         uint64 s5 = request(user1, provider1, 5, false);
 
-        assertRevealSucceeds(provider1, s4, 4, provider1Proofs[s4], ALL_ZEROS);
+        assertRevealSucceeds(
+            user1,
+            provider1,
+            s4,
+            4,
+            provider1Proofs[s4],
+            ALL_ZEROS
+        );
         assertInvariants();
 
-        assertRevealSucceeds(provider1, s1, 1, provider1Proofs[s1], ALL_ZEROS);
+        assertRevealSucceeds(
+            user1,
+            provider1,
+            s1,
+            1,
+            provider1Proofs[s1],
+            ALL_ZEROS
+        );
         assertInvariants();
 
-        assertRevealSucceeds(provider1, s2, 2, provider1Proofs[s2], ALL_ZEROS);
+        assertRevealSucceeds(
+            user2,
+            provider1,
+            s2,
+            2,
+            provider1Proofs[s2],
+            ALL_ZEROS
+        );
         assertInvariants();
 
-        assertRevealSucceeds(provider1, s5, 5, provider1Proofs[s5], ALL_ZEROS);
+        assertRevealSucceeds(
+            user1,
+            provider1,
+            s5,
+            5,
+            provider1Proofs[s5],
+            ALL_ZEROS
+        );
         assertInvariants();
     }
 
@@ -321,21 +392,13 @@ contract EntropyTest is Test, EntropyTestUtils {
         );
 
         assertRevealSucceeds(
+            user2,
             provider1,
             sequenceNumber,
             42,
             provider1Proofs[sequenceNumber],
             blockhash(1234)
         );
-
-        // You can only reveal the random number once. This isn't a feature of the contract per se, but it is
-        // the expected behavior.
-        assertRevealReverts(
-            provider1,
-            sequenceNumber,
-            42,
-            provider1Proofs[sequenceNumber]
-        );
     }
 
     function testProviderCommitmentRotation() public {
@@ -371,13 +434,15 @@ contract EntropyTest is Test, EntropyTestUtils {
         // Requests that were in-flight at the time of rotation use the commitment from the time of request
         for (uint256 i = 0; i < 10; i++) {
             assertRevealReverts(
+                user2,
                 provider1,
                 sequenceNumber1,
                 userRandom,
                 newHashChain[i]
             );
         }
         assertRevealSucceeds(
+            user2,
             provider1,
             sequenceNumber1,
             userRandom,
@@ -388,12 +453,14 @@ contract EntropyTest is Test, EntropyTestUtils {
 
         // Requests after the rotation use the new commitment
         assertRevealReverts(
+            user2,
             provider1,
             sequenceNumber3,
             userRandom,
             provider1Proofs[sequenceNumber3]
         );
         assertRevealSucceeds(
+            user2,
             provider1,
             sequenceNumber3,
             userRandom,

target_chains/ethereum/entropy_sdk/solidity/EntropyErrors.sol

@@ -20,6 +20,6 @@ library EntropyErrors {
     error IncorrectRevelation();
     // Governance message is invalid (e.g., deserialization error).
     error InvalidUpgradeMagic();
-    // Unauthorized (e.g., invalid admin or owner authorisation).
+    // The msg.sender is not allowed to invoke this call.
     error Unauthorized();
 }

target_chains/ethereum/entropy_sdk/solidity/EntropyStructs.sol

@@ -49,7 +49,7 @@ contract EntropyStructs {
         // Note that we're using a uint96 such that we have an additional 20 bytes of storage afterward for an address.
         // Although block.number returns a uint256, 96 bits should be plenty to index all of the blocks ever generated.
         uint96 blockNumber;
-
-        // TODO: store the calling contract address here and authenticate the reveal method
+        // The address that requested this random number.
+        address requester;
     }
 }