fix: Guard against malicious processors [SUP-8873]

Makefile

@@ -121,7 +121,7 @@ build-sizes: ## Builds the project and shows sizes
 
 .PHONY: test-vvv
 test-vvv: ## Runs tests with verbose output
-	forge test --match-contract SDMVW0TokenInputNoSlippageAMB1323 --evm-version cancun -vvv
+	forge test --match-test test_crossChainRebalance_updateSuperformData_allErrors --evm-version cancun -vvv
 
 .PHONY: ftest
 ftest: ## Runs tests with cancun evm version

src/interfaces/ISuperformRouterPlusAsync.sol

@@ -18,6 +18,9 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
     /// @notice thrown if the caller is not router plus
     error NOT_ROUTER_PLUS();
 
+    /// @notice thrown if the caller is not core state registry rescuer
+    error NOT_CORE_STATE_REGISTRY_RESCUER();
+
     /// @notice thrown if the rebalance to update is invalid
     error COMPLETE_REBALANCE_INVALID_TX_DATA_UPDATE();
 
@@ -44,17 +47,17 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
     /// @notice thrown to avoid processing the same rebalance payload twice
     error REBALANCE_ALREADY_PROCESSED();
 
-    /// @notice thrown when the refund proposer is invalid
-    error INVALID_PROPOSER();
+    /// @notice thrown when the refund requester is not the payload receiver
+    error INVALID_REQUESTER();
 
     /// @notice thrown when the refund payload is invalid
     error INVALID_REFUND_DATA();
 
-    /// @notice thrown when refund is already proposed
-    error REFUND_ALREADY_PROPOSED();
+    /// @notice thrown when requested refund amount is too high
+    error REQUESTED_AMOUNT_TOO_HIGH();
 
-    /// @notice thrown if the refund is still in dispute phase
-    error IN_DISPUTE_PHASE();
+    /// @notice thrown when the refund payload is already approved
+    error REFUND_ALREADY_APPROVED();
 
     //////////////////////////////////////////////////////////////
     //                       EVENTS                             //
@@ -74,6 +77,15 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
         uint256 indexed routerPlusPayloadId, address indexed refundReceiver, address refundToken, uint256 refundAmount
     );
 
+    /// @notice emitted when a refund is proposed
+    /// @param routerPlusPayloadId is the unique identifier for the payload
+    /// @param refundReceiver is the address of the user who'll receiver the refund
+    /// @param refundToken is the token to be refunded
+    /// @param refundAmount is the new refund amount
+    event refundRequested(
+        uint256 indexed routerPlusPayloadId, address indexed refundReceiver, address refundToken, uint256 refundAmount
+    );
+
     /// @notice emitted when an existing refund got disputed
     /// @param routerPlusPayloadId is the unique identifier for the payload
     /// @param disputer is the address of the user who disputed the refund
@@ -97,7 +109,6 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
         address receiver;
         address interimToken;
         uint256 amount;
-        uint256 proposedTime;
     }
 
     struct DecodedRouterPlusRebalanceCallData {
@@ -168,16 +179,12 @@ interface ISuperformRouterPlusAsync is IBaseSuperformRouterPlus {
         payable
         returns (bool rebalanceSuccessful);
 
-    /// @notice allows the receiver / disputer to protect against malicious processors
-    /// @param finalPayloadId_ is the unique identifier of the refund
-    function disputeRefund(uint256 finalPayloadId_) external;
-
-    /// @notice allows the rescuer to propose a new refund amount after a successful dispute
-    /// @param finalPayloadId_ is the unique identifier of the refund
-    /// @param refundAmount_ is the new refund amount proposed
-    function proposeRefund(uint256 finalPayloadId_, uint256 refundAmount_) external;
+    /// @notice allows the user to request a refund for the rebalance
+    /// @param routerplusPayloadId_ the router plus payload id
+    function requestRefund(uint256 routerplusPayloadId_, uint256 requestedAmount) external;
 
-    /// @notice allows the user to claim their refund post the dispute period
-    /// @param finalPayloadId_ is the unique identifier of the refund
-    function finalizeRefund(uint256 finalPayloadId_) external;
-}
+    /// @dev only callable by core state registry rescuer
+    /// @notice approves a refund for the rebalance and sends funds to the receiver
+    /// @param routerplusPayloadId_ the router plus payload id
+    function approveRefund(uint256 routerplusPayloadId_) external;
+}

src/router-plus/SuperformRouterPlusAsync.sol

@@ -35,6 +35,9 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
 
     mapping(uint256 routerPlusPayloadId => Refund) public refunds;
     mapping(uint256 routerPlusPayloadId => bool processed) public processedRebalancePayload;
+
+    mapping(uint256 routerPlusPayloadId => bool approvedRefund) public approvedRefund;
+
     //////////////////////////////////////////////////////////////
     //                       MODIFIERS                          //
     //////////////////////////////////////////////////////////////
@@ -53,6 +56,13 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
         _;
     }
 
+    modifier onlyCoreStateRegistryRescuer() {
+        if (!_hasRole(keccak256("CORE_STATE_REGISTRY_RESCUER_ROLE"), msg.sender)) {
+            revert NOT_CORE_STATE_REGISTRY_RESCUER();
+        }
+        _;
+    }
+
     //////////////////////////////////////////////////////////////
     //                      CONSTRUCTOR                         //
     //////////////////////////////////////////////////////////////
@@ -253,8 +263,8 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
             ENTIRE_SLIPPAGE * args_.amountReceivedInterimAsset
                 < ((data.expectedAmountInterimAsset * (ENTIRE_SLIPPAGE - data.slippage)))
         ) {
-            refunds[args_.routerPlusPayloadId] =
-                Refund(args_.receiverAddressSP, data.interimAsset, args_.amountReceivedInterimAsset, block.timestamp);
+
+            refunds[args_.routerPlusPayloadId] = Refund(args_.receiverAddressSP, data.interimAsset, 0);
 
             emit RefundInitiated(
                 args_.routerPlusPayloadId, args_.receiverAddressSP, data.interimAsset, args_.amountReceivedInterimAsset
@@ -415,42 +425,30 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
     }
 
     /// @inheritdoc ISuperformRouterPlusAsync
-    function disputeRefund(uint256 routerPlusPayloadId_) external override {
-        Refund storage r = refunds[routerPlusPayloadId_];
-
-        if (!(msg.sender == r.receiver || _hasRole(keccak256("CORE_STATE_REGISTRY_DISPUTER_ROLE"), msg.sender))) {
-            revert Error.NOT_VALID_DISPUTER();
-        }
-
-        if (r.proposedTime == 0 || block.timestamp > r.proposedTime + _getDelay()) revert Error.DISPUTE_TIME_ELAPSED();
-
-        /// @dev just can reset the last proposed time, since amounts should be updated again to
-        /// pass the proposedTime zero check in finalize
-        r.proposedTime = 0;
-
-        emit RefundDisputed(routerPlusPayloadId_, msg.sender);
-    }
+    function requestRefund(uint256 routerPlusPayloadId_, uint256 requestedAmount) external {
+        Refund memory r = refunds[routerPlusPayloadId_];
 
-    /// @inheritdoc ISuperformRouterPlusAsync
-    function proposeRefund(uint256 routerPlusPayloadId_, uint256 refundAmount_) external {
-        if (!_hasRole(keccak256("CORE_STATE_REGISTRY_RESCUER_ROLE"), msg.sender)) revert INVALID_PROPOSER();
+        if (msg.sender != r.receiver) revert INVALID_REQUESTER();
+        if (r.interimToken == address(0)) revert INVALID_REFUND_DATA();
 
-        Refund storage r = refunds[routerPlusPayloadId_];
+        XChainRebalanceData memory data = xChainRebalanceCallData[r.receiver][routerPlusPayloadId_];
 
-        if (r.interimToken == address(0) || r.receiver == address(0)) revert INVALID_REFUND_DATA();
-        if (r.proposedTime != 0) revert REFUND_ALREADY_PROPOSED();
+        if (requestedAmount > data.expectedAmountInterimAsset) {
+            revert REQUESTED_AMOUNT_TOO_HIGH();
+        }
 
-        r.proposedTime = block.timestamp;
-        r.amount = refundAmount_;
+        refunds[routerPlusPayloadId_] = Refund(msg.sender, data.interimAsset, requestedAmount);
 
-        emit NewRefundAmountProposed(routerPlusPayloadId_, refundAmount_);
+        emit refundRequested(routerPlusPayloadId_, msg.sender, r.interimToken, requestedAmount);
     }
 
     /// @inheritdoc ISuperformRouterPlusAsync
-    function finalizeRefund(uint256 routerPlusPayloadId_) external {
+    function approveRefund(uint256 routerPlusPayloadId_) external onlyCoreStateRegistryRescuer {
+        if (approvedRefund[routerPlusPayloadId_]) revert REFUND_ALREADY_APPROVED();
+
         Refund memory r = refunds[routerPlusPayloadId_];
 
-        if (r.proposedTime == 0 || block.timestamp <= r.proposedTime + _getDelay()) revert IN_DISPUTE_PHASE();
+        approvedRefund[routerPlusPayloadId_] = true;
 
         /// @dev deleting to prevent re-entrancy
         delete refunds[routerPlusPayloadId_];
@@ -464,15 +462,6 @@ contract SuperformRouterPlusAsync is ISuperformRouterPlusAsync, BaseSuperformRou
     //                   INTERNAL FUNCTIONS                     //
     //////////////////////////////////////////////////////////////
 
-    /// @dev returns the current dispute delay
-    function _getDelay() internal view returns (uint256) {
-        uint256 delay = superRegistry.delay();
-        if (delay == 0) {
-            revert Error.DELAY_NOT_SET();
-        }
-        return delay;
-    }
-
     function _updateSuperformData(
         MultiVaultSFData memory sfData,
         LiqRequest[] memory liqRequests,

test/fuzz/crosschain-data/adapters/HyperlaneImplementation.t.sol

@@ -107,21 +107,23 @@ contract HyperlaneImplementationTest is CommonProtocolActions {
         vm.startPrank(deployer);
         uint256 userIndex = userSeed_ % users.length;
 
+
+        vm.assume(malice_ != getContract(ETH, "CoreStateRegistry"));
+        vm.assume(malice_ != getContract(ETH, "TimelockStateRegistry"));
+        vm.assume(malice_ != getContract(ETH, "BroadcastRegistry"));
+        vm.assume(malice_ != getContract(ETH, "AsyncStateRegistry"));
+
         AMBMessage memory ambMessage;
         BroadCastAMBExtraData memory ambExtraData;
         address coreStateRegistry;
 
         (ambMessage, ambExtraData, coreStateRegistry) =
             setupBroadcastPayloadAMBData(users[userIndex], address(hyperlaneImplementation));
-
-        vm.expectRevert(Error.NOT_STATE_REGISTRY.selector);
-        vm.assume(malice_ != getContract(ETH, "CoreStateRegistry"));
-        vm.assume(malice_ != getContract(ETH, "TimelockStateRegistry"));
-        vm.assume(malice_ != getContract(ETH, "BroadcastRegistry"));
-        vm.assume(malice_ != getContract(ETH, "AsyncStateRegistry"));
+        vm.stopPrank();
 
         vm.deal(malice_, 100 ether);
         vm.prank(malice_);
+        vm.expectRevert(Error.NOT_STATE_REGISTRY.selector);
         hyperlaneImplementation.dispatchPayload{ value: 0.1 ether }(
             users[userIndex], chainIds[5], abi.encode(ambMessage), abi.encode(ambExtraData)
         );

test/mainnet/SmokeTest.Staging.t.sol

@@ -555,14 +555,13 @@ contract SmokeTestStaging is MainnetBaseSetup {
 
         for (uint256 i; i < TARGET_DEPLOYMENT_CHAINS.length; ++i) {
             uint64 chainId = TARGET_DEPLOYMENT_CHAINS[i];
-
             vm.selectFork(FORKS[chainId]);
             superFactory = SuperformFactory(getContract(chainId, "SuperformFactory"));
 
             chainId != BLAST
                 ? assertEq(superFactory.getFormImplementation(5), getContract(chainId, "ERC5115Form"))
                 : assertEq(superFactory.getFormImplementation(205), getContract(chainId, "ERC5115Form"));
-            /// @dev in blast there are 6 forms (2 without operator, 2 with wrong state registry and 2 right superforms)
+
             assertEq(superFactory.getFormCount(), chainId == LINEA ? 5 : chainId == BLAST ? 8 : 7);
             assertEq(superFactory.getFormStateRegistryId(5), 1);
         }

test/unit/payments/PaymentHelper.t.sol

@@ -569,7 +569,6 @@ contract PaymentHelperTest is ProtocolActions {
 
         assertEq(fees3, 0);
     }
-
 
     function test_estimateSingleDirectMultiVault() public view {
         /// @dev scenario: single vault withdrawal

test/unit/router-plus/SuperformRouterPlus.t.sol

@@ -1698,56 +1698,65 @@ contract SuperformRouterPlusTest is ProtocolActions {
         SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).completeCrossChainRebalance{ value: 1 ether }(completeArgs);
         vm.stopPrank();
 
-        vm.expectRevert(Error.NOT_VALID_DISPUTER.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
+        // Step 5: Request refund
 
-        vm.startPrank(deployer);
-        vm.mockCall(
-            address(getContract(SOURCE_CHAIN, "SuperRegistry")),
-            abi.encodeWithSelector(ISuperRegistry.delay.selector),
-            abi.encode(0)
-        );
-        vm.expectRevert(Error.DELAY_NOT_SET.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
-        vm.clearMockedCalls();
-
-        vm.warp(block.timestamp + 100 days);
-        vm.expectRevert(Error.DISPUTE_TIME_ELAPSED.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
-
-        vm.warp(block.timestamp - 100 days);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
+        /// @dev testing invalid requester (not receiver)
+        vm.startPrank(address(222));
+        vm.expectRevert(ISuperformRouterPlusAsync.INVALID_REQUESTER.selector);
+        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).requestRefund(1, 100);
+        vm.stopPrank();
 
-        vm.expectRevert(Error.DISPUTE_TIME_ELAPSED.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).disputeRefund(1);
+        // @dev testing refund amount exceeds expected amount
+        vm.startPrank(deployer);
+        vm.expectRevert(ISuperformRouterPlusAsync.REQUESTED_AMOUNT_TOO_HIGH.selector);
+        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).requestRefund(1, 1000e18);
         vm.stopPrank();
 
-        vm.expectRevert(ISuperformRouterPlusAsync.INVALID_PROPOSER.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).proposeRefund(1, completeArgs.amountReceivedInterimAsset);
+        /// @dev testing valid refund request
+        vm.prank(deployer);
+        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).requestRefund(1, 100);
 
-        vm.startPrank(deployer);
-        vm.expectRevert(ISuperformRouterPlusAsync.INVALID_REFUND_DATA.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).proposeRefund(2, completeArgs.amountReceivedInterimAsset);
+        (,, uint256 requestedAmount) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
+        assertEq(requestedAmount, 100);
 
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).proposeRefund(1, completeArgs.amountReceivedInterimAsset);
+        (, address refundToken,) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
+        assertEq(refundToken, address(args.interimAsset));
 
-        vm.expectRevert(ISuperformRouterPlusAsync.REFUND_ALREADY_PROPOSED.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).proposeRefund(1, completeArgs.amountReceivedInterimAsset);
+        // Step 6: Approve refund
 
-        vm.expectRevert(ISuperformRouterPlusAsync.IN_DISPUTE_PHASE.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).finalizeRefund(1);
+        /// @dev testing invalid approver (not core state registry)
+        vm.startPrank(address(1234));
+        vm.expectRevert(ISuperformRouterPlusAsync.NOT_CORE_STATE_REGISTRY_RESCUER.selector);
+        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).approveRefund(1);
+        vm.stopPrank();
 
-        (, address refundToken,,) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
+        /// @dev testing valid refund approval
         uint256 balanceBefore = MockERC20(refundToken).balanceOf(deployer);
+        uint256 routerBalanceBefore = MockERC20(refundToken).balanceOf(address(ROUTER_PLUS_ASYNC_SOURCE));
+        vm.startPrank(deployer);
+        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).approveRefund(1);
+        vm.stopPrank();
 
-        vm.warp(block.timestamp + 100 days);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).finalizeRefund(1);
         uint256 balanceAfter = MockERC20(refundToken).balanceOf(deployer);
-
         assertGt(balanceAfter, balanceBefore);
+        assertEq(MockERC20(refundToken).balanceOf(address(ROUTER_PLUS_ASYNC_SOURCE)), routerBalanceBefore - 100);
+        assertEq(MockERC20(refundToken).balanceOf(address(deployer)), balanceBefore + 100);
+
+        (, address interimToken,) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
+        assertEq(interimToken, address(0));
+
+        (, address receiver,) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
+        assertEq(receiver, address(0));
 
-        vm.expectRevert(ISuperformRouterPlusAsync.IN_DISPUTE_PHASE.selector);
-        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).finalizeRefund(1);
+        (,, uint256 updatedRequestedAmount) = SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).refunds(1);
+        assertEq(updatedRequestedAmount, 0);
+        vm.stopPrank();
+
+        /// @dev testing refund already approved
+        vm.startPrank(deployer);
+        vm.expectRevert(ISuperformRouterPlusAsync.REFUND_ALREADY_APPROVED.selector);
+        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).approveRefund(1);
+        vm.stopPrank();
     }
 
     function test_crossChainRebalance_negativeSlippage() public {
@@ -1804,6 +1813,7 @@ contract SuperformRouterPlusTest is ProtocolActions {
 
     function test_crossChainRebalance_updateSuperformData_allErrors() public {
         vm.selectFork(FORKS[SOURCE_CHAIN]);
+
         SingleVaultSFData memory sfData = SingleVaultSFData({
             superformId: superformId1,
             amount: 1e18,
@@ -1911,14 +1921,19 @@ contract SuperformRouterPlusTest is ProtocolActions {
 
         // Reset liqDstChainId
         completeArgs.liqRequests[0][0].liqDstChainId = SOURCE_CHAIN;
-
-        /// @FIXME: This line is not reacheable
-        // vm.expectRevert(ISuperformRouterPlusAsync.COMPLETE_REBALANCE_DIFFERENT_RECEIVER.selector);
-        // completeArgs.receiverAddressSP = address(0x123);
-        // SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).completeCrossChainRebalance{ value: 1 ether
-        // }(completeArgs);
         vm.stopPrank();
+        vm.startPrank(ROUTER_PLUS_SOURCE);
+        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).setXChainRebalanceCallData(address(0x123), 2, data);
+        vm.stopPrank();
+        vm.startPrank(deployer);
 
+        vm.expectRevert(ISuperformRouterPlusAsync.COMPLETE_REBALANCE_DIFFERENT_RECEIVER.selector);
+        completeArgs.routerPlusPayloadId = 2;
+        completeArgs.receiverAddressSP = address(0x123);
+        SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).completeCrossChainRebalance{ value: 1 ether }(completeArgs);
+        vm.stopPrank();
+        completeArgs.routerPlusPayloadId = 1;
+        completeArgs.receiverAddressSP = deployer;
         sfData.liqRequest.token = address(0);
 
         data = IBaseSuperformRouterPlus.XChainRebalanceData({
@@ -1939,6 +1954,7 @@ contract SuperformRouterPlusTest is ProtocolActions {
         completeArgs.routerPlusPayloadId = 2;
         vm.expectRevert(ISuperformRouterPlusAsync.COMPLETE_REBALANCE_INVALID_TX_DATA_UPDATE.selector);
         SuperformRouterPlusAsync(ROUTER_PLUS_ASYNC_SOURCE).completeCrossChainRebalance{ value: 1 ether }(completeArgs);
+        vm.stopPrank();
     }
 
     //////////////////////////////////////////////////////////////