actually _check_ the reentrant value

Uniswap · Nov 5, 2024 · 180e443 · 180e443
1 parent 4e1d696
commit 180e443
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 105 deletions.
diff --git a/snapshots/TheCompactTest.json b/snapshots/TheCompactTest.json
@@ -1,42 +1,42 @@
 {
   "basicTransfer": "56867",
-  "basicWithdrawal": "59810",
-  "batchClaim": "112477",
-  "batchClaimRegisteredWithDeposit": "112477",
-  "batchClaimRegisteredWithDepositWithWitness": "113236",
-  "batchClaimWithWitness": "113230",
-  "batchDepositAndRegisterViaPermit2": "221877",
-  "batchDepositAndRegisterWithWitnessViaPermit2": "221855",
+  "basicWithdrawal": "59992",
+  "batchClaim": "112512",
+  "batchClaimRegisteredWithDeposit": "112512",
+  "batchClaimRegisteredWithDepositWithWitness": "113271",
+  "batchClaimWithWitness": "113265",
+  "batchDepositAndRegisterViaPermit2": "222059",
+  "batchDepositAndRegisterWithWitnessViaPermit2": "222037",
   "batchTransfer": "81344",
-  "batchWithdrawal": "99729",
-  "claim": "56982",
-  "claimAndWithdraw": "73258",
-  "claimWithWitness": "59479",
-  "depositAndRegisterViaPermit2": "124247",
-  "depositBatchSingleERC20": "67868",
-  "depositBatchSingleNative": "28171",
-  "depositBatchViaPermit2NativeAndERC20": "129569",
-  "depositBatchViaPermit2SingleERC20": "104723",
-  "depositERC20AndURI": "67094",
-  "depositERC20Basic": "67120",
-  "depositERC20ViaPermit2AndURI": "98289",
+  "batchWithdrawal": "100093",
+  "claim": "56974",
+  "claimAndWithdraw": "73432",
+  "claimWithWitness": "59471",
+  "depositAndRegisterViaPermit2": "124429",
+  "depositBatchSingleERC20": "68050",
+  "depositBatchSingleNative": "28353",
+  "depositBatchViaPermit2NativeAndERC20": "129751",
+  "depositBatchViaPermit2SingleERC20": "104905",
+  "depositERC20AndURI": "67276",
+  "depositERC20Basic": "67302",
+  "depositERC20ViaPermit2AndURI": "98471",
   "depositETHAndURI": "26754",
   "depositETHBasic": "28391",
-  "qualifiedBatchClaim": "114099",
-  "qualifiedBatchClaimWithWitness": "113637",
-  "qualifiedClaim": "60414",
-  "qualifiedClaimWithWitness": "59106",
-  "qualifiedSplitBatchClaim": "141456",
-  "qualifiedSplitBatchClaimWithWitness": "141522",
+  "qualifiedBatchClaim": "114134",
+  "qualifiedBatchClaimWithWitness": "113672",
+  "qualifiedClaim": "60406",
+  "qualifiedClaimWithWitness": "59098",
+  "qualifiedSplitBatchClaim": "141486",
+  "qualifiedSplitBatchClaimWithWitness": "141552",
   "qualifiedSplitClaim": "86977",
   "qualifiedSplitClaimWithWitness": "87452",
   "register": "25379",
-  "splitBatchClaim": "140758",
-  "splitBatchClaimWithWitness": "140719",
-  "splitBatchTransfer": "110582",
-  "splitBatchWithdrawal": "139745",
+  "splitBatchClaim": "140788",
+  "splitBatchClaimWithWitness": "140749",
+  "splitBatchTransfer": "110652",
+  "splitBatchWithdrawal": "140361",
   "splitClaim": "86751",
   "splitClaimWithWitness": "86232",
-  "splitTransfer": "82439",
-  "splitWithdrawal": "93363"
+  "splitTransfer": "82482",
+  "splitWithdrawal": "93770"
 }
diff --git a/src/interfaces/ITheCompact.sol b/src/interfaces/ITheCompact.sol
@@ -567,4 +567,5 @@ interface ITheCompact {
     error InvalidDepositBalanceChange();
     error Permit2CallFailed();
     error InvalidRegistrationDuration(uint256 duration);
+    error ReentrantCall(address existingCaller);
 }
diff --git a/src/lib/ComponentLib.sol b/src/lib/ComponentLib.sol
@@ -44,23 +44,8 @@ library ComponentLib {
      * @return          Whether the transfer was successfully processed.
      */
     function processSplitTransfer(SplitTransfer calldata transfer, function(address, address, uint256, uint256) internal returns (bool) operation) internal returns (bool) {
-        // Navigate to the split components in calldata.
-        SplitComponent[] calldata recipients = transfer.recipients;
-
-        // Retrieve the resource lock ID and the total number of components.
-        uint256 id = transfer.id;
-        uint256 totalSplits = recipients.length;
-
-        unchecked {
-            // Iterate over each additional component in calldata.
-            for (uint256 i = 0; i < totalSplits; ++i) {
-                // Navigate to location of the component in calldata.
-                SplitComponent calldata component = recipients[i];
-
-                // Perform the transfer or withdrawal for the portion.
-                operation(msg.sender, component.claimant, id, component.amount);
-            }
-        }
+        // Process the transfer for each split component.
+        _processSplitTransferComponents(transfer.recipients, transfer.id, operation);
 
         return true;
     }
@@ -104,9 +89,6 @@ library ComponentLib {
      * @return          Whether the transfer was successfully processed.
      */
     function performSplitBatchTransfer(SplitBatchTransfer calldata transfer, function(address, address, uint256, uint256) internal returns (bool) operation) internal returns (bool) {
-        // Declare a variable for tracking the id of each component.
-        uint256 id;
-
         // Navigate to the split batch components array in calldata.
         SplitByIdComponent[] calldata transfers = transfer.transfers;
 
@@ -119,23 +101,8 @@ library ComponentLib {
                 // Navigate to location of the component in calldata.
                 SplitByIdComponent calldata component = transfers[i];
 
-                // Retrieve the id for the component.
-                id = component.id;
-
-                // Navigate to location of component portions in calldata.
-                SplitComponent[] calldata portions = component.portions;
-
-                // Retrieve the total number of portions on the component.
-                uint256 totalPortions = portions.length;
-
-                // Iterate over each portion in calldata.
-                for (uint256 j = 0; j < totalPortions; ++j) {
-                    // Navigate to the location of the portion in calldata.
-                    SplitComponent calldata portion = portions[j];
-
-                    // Perform the transfer or withdrawal for the portion.
-                    operation(msg.sender, portion.claimant, id, portion.amount);
-                }
+                // Process transfer for each split component in the set.
+                _processSplitTransferComponents(component.portions, component.id, operation);
             }
         }
 
@@ -249,20 +216,13 @@ library ComponentLib {
 
         // Revert if the batch is empty.
         uint256 totalClaims = claims.length;
-        assembly ("memory-safe") {
-            if iszero(totalClaims) {
-                // revert InvalidBatchAllocation()
-                mstore(0, 0x3a03d3bb)
-                revert(0x1c, 0x04)
-            }
-        }
 
         // Process first claim and initialize error tracking.
         // NOTE: many of the bounds checks on these array accesses can be skipped as an optimization
         BatchClaimComponent calldata component = claims[0];
         uint256 id = component.id;
         uint256 amount = component.amount;
-        uint256 errorBuffer = component.allocatedAmount.allocationExceededOrScopeNotMultichain(amount, id, sponsorDomainSeparator).asUint256();
+        uint256 errorBuffer = (totalClaims == 0).or(component.allocatedAmount.allocationExceededOrScopeNotMultichain(amount, id, sponsorDomainSeparator)).asUint256();
 
         // Execute transfer or withdrawal for first claim.
         operation(sponsor, claimant, id, amount);
@@ -278,21 +238,7 @@ library ComponentLib {
                 operation(sponsor, claimant, id, amount);
             }
 
-            // If any errors occurred, identify specific failures and revert.
-            if (errorBuffer.asBool()) {
-                for (uint256 i = 0; i < totalClaims; ++i) {
-                    component = claims[i];
-                    component.amount.withinAllocated(component.allocatedAmount);
-                    id = component.id;
-                    sponsorDomainSeparator.ensureValidScope(component.id);
-                }
-
-                assembly ("memory-safe") {
-                    // revert InvalidBatchAllocation()
-                    mstore(0, 0x3a03d3bb)
-                    revert(0x1c, 0x04)
-                }
-            }
+            _revertWithInvalidBatchAllocationIfError(errorBuffer);
         }
 
         return true;
@@ -358,18 +304,8 @@ library ComponentLib {
                 claimComponent.portions.verifyAndProcessSplitComponents(sponsor, id, claimComponent.allocatedAmount, operation);
             }
 
-            // If any errors occurred, identify specific scope failures and revert.
-            if (errorBuffer.asBool()) {
-                for (uint256 i = 0; i < totalClaims; ++i) {
-                    sponsorDomainSeparator.ensureValidScope(claims[i].id);
-                }
-
-                assembly ("memory-safe") {
-                    // revert InvalidBatchAllocation()
-                    mstore(0, 0x3a03d3bb)
-                    revert(0x1c, 0x04)
-                }
-            }
+            // Revert if any errors occurred.
+            _revertWithInvalidBatchAllocationIfError(errorBuffer);
         }
 
         return true;
@@ -429,4 +365,44 @@ library ComponentLib {
 
         return true;
     }
+
+    /**
+     * @notice Private function for performing a set of split transfers or withdrawals
+     * given an array of split components and an ID for an associated resource lock.
+     * Executes the transfer or withdrawal operation targeting multiple recipients.
+     * @param recipients A SplitComponent struct array containing split transfer details.
+     * @param id         The ERC6909 token identifier of the resource lock.
+     * @param operation  Function pointer to either _release or _withdraw for executing the claim.
+     */
+    function _processSplitTransferComponents(SplitComponent[] calldata recipients, uint256 id, function(address, address, uint256, uint256) internal returns (bool) operation) private {
+        // Retrieve the total number of components.
+        uint256 totalSplits = recipients.length;
+
+        unchecked {
+            // Iterate over each additional component in calldata.
+            for (uint256 i = 0; i < totalSplits; ++i) {
+                // Navigate to location of the component in calldata.
+                SplitComponent calldata component = recipients[i];
+
+                // Perform the transfer or withdrawal for the portion.
+                operation(msg.sender, component.claimant, id, component.amount);
+            }
+        }
+    }
+
+    /**
+     * @notice Private pure function that reverts with an InvalidBatchAllocation error
+     * if an error buffer is nonzero.
+     * @param errorBuffer The error buffer to check.
+     */
+    function _revertWithInvalidBatchAllocationIfError(uint256 errorBuffer) private pure {
+        // Revert if any errors occurred.
+        assembly ("memory-safe") {
+            if errorBuffer {
+                // revert InvalidBatchAllocation()
+                mstore(0, 0x3a03d3bb)
+                revert(0x1c, 0x04)
+            }
+        }
+    }
 }
diff --git a/src/lib/ConstructorLogic.sol b/src/lib/ConstructorLogic.sol
@@ -57,10 +57,27 @@ contract ConstructorLogic is Tstorish {
 
     /**
      * @notice Internal function to set the reentrancy guard using either TSTORE or SSTORE.
-     * Called as part of functions that require reentrancy protection.
+     * Called as part of functions that require reentrancy protection. Reverts if called
+     * again before the reentrancy guard has been cleared.
+     * @dev Note that the caller is set to the value; this enables external contracts to
+     * ascertain the account originating the ongoing call while handling the call as long
+     * as exttload is available.
      */
     function _setReentrancyGuard() internal {
-        _setTstorish(_REENTRANCY_GUARD_SLOT, 1);
+        uint256 entered = _getTstorish(_REENTRANCY_GUARD_SLOT);
+
+        assembly ("memory-safe") {
+            if entered {
+                // revert ReentrantCall(address existingCaller)
+                mstore(0, 0xf57c448b)
+                mstore(0x20, entered)
+                revert(0x1c, 0x24)
+            }
+
+            entered := caller()
+        }
+
+        _setTstorish(_REENTRANCY_GUARD_SLOT, entered);
     }
 
     /**