Merge pull request #27 from propeller-heads/router/tnl/ENG-4046-static-call-verifier

feat: Perform staticcall to CallbackVerifier

foundry/interfaces/ICallbackVerifier.sol

@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.28;
+
+interface ICallbackVerifier {
+    error UnauthorizedCaller(string exchange, address sender);
+
+    /**
+     * @dev This method should revert if the sender is not a verified sender of the exchange.
+     */
+    function verifyCallback(address sender, bytes calldata data)
+        external
+        returns (
+            uint256 amountOwed,
+            uint256 amountReceived,
+            address tokenOwed,
+            uint16 dataOffset
+        );
+}

foundry/src/CallbackVerificationDispatcher.sol

@@ -1,5 +1,10 @@
 // SPDX-License-Identifier: UNLICENSED
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.28;
+
+import "@interfaces/ICallbackVerifier.sol";
+
+error CallbackVerificationDispatcher__UnapprovedVerifier();
+error CallbackVerificationDispatcher__NonContractVerifier();
 
 /**
  * @title Dispatch callback verification to external contracts
@@ -12,4 +17,88 @@ pragma solidity ^0.8.0;
  */
 contract CallbackVerificationDispatcher {
     mapping(address => bool) public callbackVerifiers;
+
+    event CallbackVerifierSet(address indexed callbackVerifier);
+    event CallbackVerifierRemoved(address indexed callbackVerifier);
+
+    /**
+     * @dev Adds or replaces an approved callback verifier contract address if it is a
+     *  contract.
+     * @param target address of the callback verifier contract
+     */
+    function _setCallbackVerifier(address target) internal {
+        if (target.code.length == 0) {
+            revert CallbackVerificationDispatcher__NonContractVerifier();
+        }
+        callbackVerifiers[target] = true;
+        emit CallbackVerifierSet(target);
+    }
+
+    /**
+     * @dev Removes an approved callback verifier contract address
+     * @param target address of the callback verifier contract
+     */
+    function _removeCallbackVerifier(address target) internal {
+        delete callbackVerifiers[target];
+        emit CallbackVerifierRemoved(target);
+    }
+
+    /**
+     * @dev Calls a callback verifier. This should revert if the callback verification fails.
+     */
+    // slither-disable-next-line dead-code
+    function _callVerifyCallback(bytes calldata data)
+        internal
+        returns (
+            uint256 amountOwed,
+            uint256 amountReceived,
+            address tokenOwed,
+            uint16 dataOffset
+        )
+    {
+        address verifier;
+        bytes4 decodedSelector;
+        bytes memory verifierData;
+
+        (verifier, decodedSelector, verifierData) =
+            _decodeVerifierAndSelector(data);
+
+        if (!callbackVerifiers[verifier]) {
+            revert CallbackVerificationDispatcher__UnapprovedVerifier();
+        }
+
+        bytes4 selector = decodedSelector == bytes4(0)
+            ? ICallbackVerifier.verifyCallback.selector
+            : decodedSelector;
+
+        address sender = msg.sender;
+
+        // slither-disable-next-line low-level-calls
+        (bool success, bytes memory result) = verifier.staticcall(
+            abi.encodeWithSelector(selector, sender, verifierData)
+        );
+
+        if (!success) {
+            if (result.length > 0) {
+                revert(string(result));
+            } else {
+                revert("Callback verification failed");
+            }
+        }
+
+        (amountOwed, amountReceived, tokenOwed, dataOffset) =
+            abi.decode(result, (uint256, uint256, address, uint16));
+    }
+
+    // slither-disable-next-line dead-code
+    function _decodeVerifierAndSelector(bytes calldata data)
+        internal
+        pure
+        returns (address verifier, bytes4 selector, bytes memory verifierData)
+    {
+        require(data.length >= 20, "Invalid data length");
+        verifier = address(uint160(bytes20(data[:20])));
+        selector = bytes4(data[20:24]);
+        verifierData = data[24:];
+    }
 }

foundry/src/TychoRouter.sol

@@ -11,7 +11,6 @@ import "@openzeppelin/contracts/utils/Pausable.sol";
 
 error TychoRouter__WithdrawalFailed();
 error TychoRouter__AddressZero();
-error TychoRouter__NonContractVerifier();
 
 contract TychoRouter is
     AccessControl,
@@ -48,7 +47,6 @@ contract TychoRouter is
         address indexed oldFeeReceiver, address indexed newFeeReceiver
     );
     event FeeSet(uint256 indexed oldFee, uint256 indexed newFee);
-    event CallbackVerifierSet(address indexed callbackVerifier);
 
     constructor(address _permit2) {
         permit2 = IAllowanceTransfer(_permit2);
@@ -139,9 +137,7 @@ contract TychoRouter is
         external
         onlyRole(EXECUTOR_SETTER_ROLE)
     {
-        if (target.code.length == 0) revert TychoRouter__NonContractVerifier();
+        _setCallbackVerifier(target);
-        callbackVerifiers[target] = true;
-        emit CallbackVerifierSet(target);
     }
 
     /**
@@ -152,7 +148,7 @@ contract TychoRouter is
         external
         onlyRole(EXECUTOR_SETTER_ROLE)
     {
-        delete callbackVerifiers[target];
+        _removeCallbackVerifier(target);
     }
 
     /**

foundry/test/CallbackVerificationDispatcher.t.sol

@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.28;
+
+import "@src/CallbackVerificationDispatcher.sol";
+import "./TychoRouterTestSetup.sol";
+
+contract CallbackVerificationDispatcherExposed is
+    CallbackVerificationDispatcher
+{
+    function exposedCallVerifier(bytes calldata data)
+        external
+        returns (
+            uint256 amountOwed,
+            uint256 amountReceived,
+            address tokenOwed,
+            uint16 dataOffset
+        )
+    {
+        return _callVerifyCallback(data);
+    }
+
+    function exposedDecodeVerifierAndSelector(bytes calldata data)
+        external
+        pure
+        returns (address executor, bytes4 selector, bytes memory protocolData)
+    {
+        return _decodeVerifierAndSelector(data);
+    }
+
+    function exposedSetCallbackVerifier(address target) external {
+        _setCallbackVerifier(target);
+    }
+
+    function exposedRemoveCallbackVerifier(address target) external {
+        _removeCallbackVerifier(target);
+    }
+}
+
+contract CallbackVerificationDispatcherTest is Constants {
+    CallbackVerificationDispatcherExposed dispatcherExposed;
+
+    event CallbackVerifierSet(address indexed callbackVerifier);
+    event CallbackVerifierRemoved(address indexed callbackVerifier);
+
+    function setUp() public {
+        uint256 forkBlock = 20673900;
+        vm.createSelectFork(vm.rpcUrl("mainnet"), forkBlock);
+        dispatcherExposed = new CallbackVerificationDispatcherExposed();
+        deal(WETH_ADDR, address(dispatcherExposed), 15 ether);
+        deployDummyContract();
+    }
+
+    function testSetValidVerifier() public {
+        vm.expectEmit();
+        // Define the event we expect to be emitted at the next step
+        emit CallbackVerifierSet(DUMMY);
+        dispatcherExposed.exposedSetCallbackVerifier(DUMMY);
+        assert(dispatcherExposed.callbackVerifiers(DUMMY) == true);
+    }
+
+    function testRemoveVerifier() public {
+        dispatcherExposed.exposedSetCallbackVerifier(DUMMY);
+        vm.expectEmit();
+        // Define the event we expect to be emitted at the next step
+        emit CallbackVerifierRemoved(DUMMY);
+        dispatcherExposed.exposedRemoveCallbackVerifier(DUMMY);
+        assert(dispatcherExposed.callbackVerifiers(DUMMY) == false);
+    }
+
+    function testRemoveUnSetVerifier() public {
+        dispatcherExposed.exposedRemoveCallbackVerifier(BOB);
+        assert(dispatcherExposed.callbackVerifiers(BOB) == false);
+    }
+
+    function testSetVerifierNonContract() public {
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                CallbackVerificationDispatcher__NonContractVerifier.selector
+            )
+        );
+        dispatcherExposed.exposedSetCallbackVerifier(BOB);
+    }
+
+    function testCallVerifierSuccess() public {
+        // For this test, we can use any callback verifier and any calldata that we
+        // know works for this verifier. We don't care about which calldata/executor,
+        // since we are only testing the functionality of the staticcall and not
+        // the inner verifier.
+        // Thus, this test case designed from scratch using previously-deployed
+        // Maverick callback verifier. Looking at the code, we can easily design
+        // passing calldata.
+        dispatcherExposed.exposedSetCallbackVerifier(
+            address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8)
+        );
+        bytes memory data =
+            hex"2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e876b20f8a0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001A0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48";
+        vm.startPrank(address(0xD0b2F5018B5D22759724af6d4281AC0B13266360));
+        (
+            uint256 amountOwed,
+            uint256 amountReceived,
+            address tokenOwed,
+            uint16 dataOffset
+        ) = dispatcherExposed.exposedCallVerifier(data);
+        vm.stopPrank();
+
+        // The values themselves are irrelevant, we just need to make sure that we
+        // correctly parse the expected output of the existing Maverick verifier
+        assert(amountOwed == 1);
+        assert(amountReceived == 1);
+        assert(tokenOwed == address(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48));
+        assert(dataOffset == 148);
+    }
+
+    function testCallVerifierNoSelector() public {
+        // This test is exactly the same as testCallVerifierSuccess, except that the
+        // fn selector is not explicitly passed. The test should still pass using the
+        // default selector.
+        dispatcherExposed.exposedSetCallbackVerifier(
+            address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8)
+        );
+
+        // Pass all-zero selector. This should default to the verifyCallback selector
+        bytes memory data =
+            hex"2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001A0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48";
+        vm.startPrank(address(0xD0b2F5018B5D22759724af6d4281AC0B13266360));
+        (
+            uint256 amountOwed,
+            uint256 amountReceived,
+            address tokenOwed,
+            uint16 dataOffset
+        ) = dispatcherExposed.exposedCallVerifier(data);
+        vm.stopPrank();
+
+        // The values themselves are irrelevant, we just need to make sure that we
+        // correctly parse the expected output of the existing Maverick verifier
+        assert(amountOwed == 1);
+        assert(amountReceived == 1);
+        assert(tokenOwed == address(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48));
+        assert(dataOffset == 148);
+    }
+
+    function testCallVerifierBadSelector() public {
+        // A bad selector is provided to an approved executor - causing the call
+        // itself to fail. Make sure this actually reverts.
+        dispatcherExposed.exposedSetCallbackVerifier(
+            address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8)
+        );
+        vm.startPrank(address(0xD0b2F5018B5D22759724af6d4281AC0B13266360));
+        bytes memory data =
+            hex"2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8aa0000000000";
+        vm.expectRevert(bytes("Callback verification failed"));
+        dispatcherExposed.exposedCallVerifier(data);
+        vm.stopPrank();
+    }
+
+    function testCallVerifierParseRevertMessage() public {
+        // Verification should fail because caller is not a Maverick pool
+        // Check that we correctly parse the revert message
+        dispatcherExposed.exposedSetCallbackVerifier(
+            address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8)
+        );
+        bytes memory data =
+            hex"2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8000000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001A0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48";
+        vm.expectRevert(
+            abi.encodeWithSignature(
+                "Error(string)", "Must call from a Maverick Factory Pool"
+            )
+        );
+        dispatcherExposed.exposedCallVerifier(data);
+    }
+
+    function testCallVerifierUnapprovedVerifier() public {
+        bytes memory data =
+            hex"5d622C9053b8FFB1B3465495C8a42E603632bA70aabbccdd1111111111111111";
+        vm.expectRevert();
+        dispatcherExposed.exposedCallVerifier(data);
+    }
+
+    function testDecodeVerifierAndSelector() public {
+        bytes memory data =
+            hex"2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e876b20f8aA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48";
+        (address executor, bytes4 selector, bytes memory verifierData) =
+            dispatcherExposed.exposedDecodeVerifierAndSelector(data);
+        assert(executor == address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8));
+        assert(selector == bytes4(0x76b20f8a));
+        // Direct bytes comparison not supported - must use keccak
+        assert(
+            keccak256(verifierData)
+                == keccak256(hex"A0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48")
+        );
+    }
+}

foundry/test/TychoRouter.t.sol

@@ -26,6 +26,14 @@ contract TychoRouterTest is TychoRouterTestSetup {
         assert(tychoRouter.executors(DUMMY) == true);
     }
 
+    function testRemoveExecutorValidRole() public {
+        vm.startPrank(executorSetter);
+        tychoRouter.setExecutor(DUMMY);
+        tychoRouter.removeExecutor(DUMMY);
+        vm.stopPrank();
+        assert(tychoRouter.executors(DUMMY) == false);
+    }
+
     function testRemoveExecutorMissingSetterRole() public {
         vm.expectRevert();
         tychoRouter.removeExecutor(BOB);
@@ -36,19 +44,14 @@ contract TychoRouterTest is TychoRouterTestSetup {
         tychoRouter.setExecutor(DUMMY);
     }
 
-    function testSetValidVerifier() public {
+    function testSetVerifierValidRole() public {
         vm.startPrank(executorSetter);
-        vm.expectEmit();
-        // Define the event we expect to be emitted at the next step
-        emit CallbackVerifierSet(DUMMY);
-
         tychoRouter.setCallbackVerifier(DUMMY);
         vm.stopPrank();
-
         assert(tychoRouter.callbackVerifiers(DUMMY) == true);
     }
 
-    function testRemoveVerifier() public {
+    function testRemoveVerifierValidRole() public {
         vm.startPrank(executorSetter);
         tychoRouter.setCallbackVerifier(DUMMY);
         tychoRouter.removeCallbackVerifier(DUMMY);
@@ -56,13 +59,6 @@ contract TychoRouterTest is TychoRouterTestSetup {
         assert(tychoRouter.callbackVerifiers(DUMMY) == false);
     }
 
-    function testRemoveUnSetVerifier() public {
-        vm.startPrank(executorSetter);
-        tychoRouter.removeCallbackVerifier(BOB);
-        vm.stopPrank();
-        assert(tychoRouter.callbackVerifiers(BOB) == false);
-    }
-
     function testRemoveVerifierMissingSetterRole() public {
         vm.expectRevert();
         tychoRouter.removeCallbackVerifier(BOB);
@@ -73,15 +69,6 @@ contract TychoRouterTest is TychoRouterTestSetup {
         tychoRouter.setCallbackVerifier(DUMMY);
     }
 
-    function testSetVerifierNonContract() public {
-        vm.startPrank(executorSetter);
-        vm.expectRevert(
-            abi.encodeWithSelector(TychoRouter__NonContractVerifier.selector)
-        );
-        tychoRouter.setCallbackVerifier(BOB);
-        vm.stopPrank();
-    }
-
     function testWithdrawNative() public {
         vm.startPrank(FUND_RESCUER);
         // Send 100 ether to tychoRouter