fix(SpokePool): Enforce exclusivity consistently (#649)

* fix(SpokePool): Apply exclusivity consistently The new relative exclusivity check has not been propagated to fillV3RelayWithUpdatedDeposit(). Identified via test case failures in the relayer. Signed-off-by: Paul <[email protected]> * Also check on slow fill requests * Update contracts/SpokePool.sol * lint * Update * Add pure * Fix * Add tests --------- Signed-off-by: Paul <[email protected]>
across-protocol · Oct 30, 2024 · 05302cb · 05302cb
1 parent 193d26c
commit 05302cb
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 8 deletions.
diff --git a/contracts/SpokePool.sol b/contracts/SpokePool.sol
@@ -319,7 +319,7 @@ abstract contract SpokePool is
     /**
      * @notice Pauses deposit-related functions. This is intended to be used if this contract is deprecated or when
      * something goes awry.
-     * @dev Affects `deposit()` but not `speedUpDeposit()`, so that existing deposits can be sped up and still
+     * @dev Affects `deposit()` but not `speedUpV3Deposit()`, so that existing deposits can be sped up and still
      * relayed.
      * @param pause true if the call is meant to pause the system, false if the call is meant to unpause it.
      */
@@ -839,9 +839,8 @@ abstract contract SpokePool is
         // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right
         // to fill the relay.
         if (
-            relayData.exclusiveRelayer != msg.sender &&
-            relayData.exclusivityDeadline >= getCurrentTime() &&
-            relayData.exclusiveRelayer != address(0)
+            _fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, uint32(getCurrentTime())) &&
+            relayData.exclusiveRelayer != msg.sender
         ) {
             revert NotExclusiveRelayer();
         }
@@ -885,7 +884,10 @@ abstract contract SpokePool is
     ) public override nonReentrant unpausedFills {
         // Exclusivity deadline is inclusive and is the latest timestamp that the exclusive relayer has sole right
         // to fill the relay.
-        if (relayData.exclusivityDeadline >= getCurrentTime() && relayData.exclusiveRelayer != msg.sender) {
+        if (
+            _fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, uint32(getCurrentTime())) &&
+            relayData.exclusiveRelayer != msg.sender
+        ) {
             revert NotExclusiveRelayer();
         }
 
@@ -927,12 +929,15 @@ abstract contract SpokePool is
      * then Across will not include a slow fill for the intended deposit.
      */
     function requestV3SlowFill(V3RelayData calldata relayData) public override nonReentrant unpausedFills {
+        uint32 currentTime = uint32(getCurrentTime());
         // If a depositor has set an exclusivity deadline, then only the exclusive relayer should be able to
         // fast fill within this deadline. Moreover, the depositor should expect to get *fast* filled within
         // this deadline, not slow filled. As a simplifying assumption, we will not allow slow fills to be requested
         // during this exclusivity period.
-        if (relayData.exclusivityDeadline >= getCurrentTime()) revert NoSlowFillsInExclusivityWindow();
-        if (relayData.fillDeadline < getCurrentTime()) revert ExpiredFillDeadline();
+        if (_fillIsExclusive(relayData.exclusiveRelayer, relayData.exclusivityDeadline, currentTime)) {
+            revert NoSlowFillsInExclusivityWindow();
+        }
+        if (relayData.fillDeadline < currentTime) revert ExpiredFillDeadline();
 
         bytes32 relayHash = _getV3RelayHash(relayData);
         if (fillStatuses[relayHash] != uint256(FillStatus.Unfilled)) revert InvalidSlowFillRequest();
@@ -1408,6 +1413,15 @@ abstract contract SpokePool is
         }
     }
 
+    // Determine whether the combination of exlcusiveRelayer and exclusivityDeadline implies active exclusivity.
+    function _fillIsExclusive(
+        address exclusiveRelayer,
+        uint32 exclusivityDeadline,
+        uint32 currentTime
+    ) internal pure returns (bool) {
+        return exclusivityDeadline >= currentTime && exclusiveRelayer != address(0);
+    }
+
     // Implementing contract needs to override this to ensure that only the appropriate cross chain admin can execute
     // certain admin functions. For L2 contracts, the cross chain admin refers to some L1 address or contract, and for
     // L1, this would just be the same admin of the HubPool.

diff --git a/contracts/interfaces/V3SpokePoolInterface.sol b/contracts/interfaces/V3SpokePoolInterface.sol
@@ -222,7 +222,6 @@ interface V3SpokePoolInterface {
     error DisabledRoute();
     error InvalidQuoteTimestamp();
     error InvalidFillDeadline();
-    error InvalidExclusiveRelayer();
     error InvalidExclusivityDeadline();
     error MsgValueDoesNotMatchInputAmount();
     error NotExclusiveRelayer();

diff --git a/test/evm/hardhat/SpokePool.Relay.ts b/test/evm/hardhat/SpokePool.Relay.ts
@@ -413,6 +413,23 @@ describe("SpokePool Relayer Logic", async function () {
           updatedMessage
         );
       });
+      it("in absence of exclusivity", async function () {
+        // Clock drift between spokes can mean exclusivityDeadline is in future even when no exclusivity was applied.
+        await spokePool.setCurrentTime(relayData.exclusivityDeadline - 1);
+        await expect(
+          spokePool.connect(relayer).fillV3RelayWithUpdatedDeposit(
+            {
+              ...relayData,
+              exclusiveRelayer: consts.zeroAddress,
+            },
+            consts.repaymentChainId,
+            updatedOutputAmount,
+            updatedRecipient,
+            updatedMessage,
+            signature
+          )
+        ).to.emit(spokePool, "FilledV3Relay");
+      });
       it("must be exclusive relayer before exclusivity deadline", async function () {
         const _relayData = {
           ...relayData,

diff --git a/test/evm/hardhat/SpokePool.SlowRelay.ts b/test/evm/hardhat/SpokePool.SlowRelay.ts
@@ -51,6 +51,13 @@ describe("SpokePool Slow Relay Logic", async function () {
       relayData.fillDeadline = (await spokePool.getCurrentTime()).sub(1);
       await expect(spokePool.connect(relayer).requestV3SlowFill(relayData)).to.be.revertedWith("ExpiredFillDeadline");
     });
+    it("in absence of exclusivity", async function () {
+      // Clock drift between spokes can mean exclusivityDeadline is in future even when no exclusivity was applied.
+      await spokePool.setCurrentTime(relayData.exclusivityDeadline - 1);
+      await expect(
+        spokePool.connect(relayer).requestV3SlowFill({ ...relayData, exclusiveRelayer: consts.zeroAddress })
+      ).to.emit(spokePool, "RequestedV3SlowFill");
+    });
     it("during exclusivity deadline", async function () {
       await spokePool.setCurrentTime(relayData.exclusivityDeadline);
       await expect(spokePool.connect(relayer).requestV3SlowFill(relayData)).to.be.revertedWith(