From ad0748e9c3b2431ae29be8477534853029efa27d Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Fri, 24 Jan 2025 16:55:21 -0500 Subject: [PATCH 1/3] feat: Perform staticcall to CallbackVerifier --- foundry/interfaces/ICallbackVerifier.sol | 18 ++ .../src/CallbackVerificationDispatcher.sol | 66 ++++++- .../test/CallbackVerificationDispatcher.t.sol | 163 ++++++++++++++++++ 3 files changed, 246 insertions(+), 1 deletion(-) create mode 100644 foundry/interfaces/ICallbackVerifier.sol create mode 100644 foundry/test/CallbackVerificationDispatcher.t.sol diff --git a/foundry/interfaces/ICallbackVerifier.sol b/foundry/interfaces/ICallbackVerifier.sol new file mode 100644 index 0000000..3054132 --- /dev/null +++ b/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 + ); +} diff --git a/foundry/src/CallbackVerificationDispatcher.sol b/foundry/src/CallbackVerificationDispatcher.sol index 89e05d1..072dc93 100644 --- a/foundry/src/CallbackVerificationDispatcher.sol +++ b/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,63 @@ pragma solidity ^0.8.0; */ contract CallbackVerificationDispatcher { mapping(address => bool) public callbackVerifiers; + + /** + * @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:]; + } } diff --git a/foundry/test/CallbackVerificationDispatcher.t.sol b/foundry/test/CallbackVerificationDispatcher.t.sol new file mode 100644 index 0000000..7d2fcae --- /dev/null +++ b/foundry/test/CallbackVerificationDispatcher.t.sol @@ -0,0 +1,163 @@ +// 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 exposedSetVerifier(address target) external { + callbackVerifiers[target] = true; + } + + function exposedRemoveVerifier(address target) external {} +} + +contract CallbackVerificationDispatcherTest is Constants { + CallbackVerificationDispatcherExposed dispatcherExposed; + + event VerifierSet(address indexed executor); + event VerifierRemoved(address indexed executor); + + function setUp() public { + uint256 forkBlock = 20673900; + vm.createSelectFork(vm.rpcUrl("mainnet"), forkBlock); + dispatcherExposed = new CallbackVerificationDispatcherExposed(); + deal(WETH_ADDR, address(dispatcherExposed), 15 ether); + deployDummyContract(); + } + + 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.exposedSetVerifier( + 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 { + // 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.exposedSetVerifier( + 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.exposedSetVerifier( + 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.exposedSetVerifier( + 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") + ); + } +} From 8ef061fd758242f34ea562e3861aeb6a3d9377d7 Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Fri, 24 Jan 2025 17:15:12 -0500 Subject: [PATCH 2/3] refactor: Move code check to CallbackVerificationDispatcher [copied from exact same reasoning with execution code-checking] I was inspired to do this because, when disabling the slither check for the staticcall when calling the callback verifier, I realized it's not clear from the same contract that we have already checked for contract code existence when setting the verifier. This made me feel uneasy, as this contract can then not stand alone and must rely on the higher level contract to safely check for code existence, otherwise the staticcall is unsafe. Keeping this logic in a separate contract seems error-prone to me, as we may remove the check for code existence without immediately realizing the implications of doing so. For this reason I have organized it as follows: - Logic/tests relating to proper roles/access control in the main TychoRouter. - Lower-level logic/tests that check contract validity before setting the callback verifier in the CallbackVerificationDispatcher --- .../src/CallbackVerificationDispatcher.sol | 25 +++++++++ foundry/src/TychoRouter.sol | 8 +-- .../test/CallbackVerificationDispatcher.t.sol | 51 +++++++++++++++---- foundry/test/TychoRouter.t.sol | 33 ++++-------- 4 files changed, 79 insertions(+), 38 deletions(-) diff --git a/foundry/src/CallbackVerificationDispatcher.sol b/foundry/src/CallbackVerificationDispatcher.sol index 072dc93..0e2a058 100644 --- a/foundry/src/CallbackVerificationDispatcher.sol +++ b/foundry/src/CallbackVerificationDispatcher.sol @@ -18,6 +18,31 @@ error CallbackVerificationDispatcher__NonContractVerifier(); 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. */ diff --git a/foundry/src/TychoRouter.sol b/foundry/src/TychoRouter.sol index 44d7752..bf4ba36 100644 --- a/foundry/src/TychoRouter.sol +++ b/foundry/src/TychoRouter.sol @@ -10,7 +10,6 @@ import "./CallbackVerificationDispatcher.sol"; error TychoRouter__WithdrawalFailed(); error TychoRouter__AddressZero(); -error TychoRouter__NonContractVerifier(); contract TychoRouter is AccessControl, @@ -44,7 +43,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); @@ -121,9 +119,7 @@ contract TychoRouter is external onlyRole(EXECUTOR_SETTER_ROLE) { - if (target.code.length == 0) revert TychoRouter__NonContractVerifier(); - callbackVerifiers[target] = true; - emit CallbackVerifierSet(target); + _setCallbackVerifier(target); } /** @@ -134,7 +130,7 @@ contract TychoRouter is external onlyRole(EXECUTOR_SETTER_ROLE) { - delete callbackVerifiers[target]; + _removeCallbackVerifier(target); } /** diff --git a/foundry/test/CallbackVerificationDispatcher.t.sol b/foundry/test/CallbackVerificationDispatcher.t.sol index 7d2fcae..308fc8f 100644 --- a/foundry/test/CallbackVerificationDispatcher.t.sol +++ b/foundry/test/CallbackVerificationDispatcher.t.sol @@ -27,18 +27,20 @@ contract CallbackVerificationDispatcherExposed is return _decodeVerifierAndSelector(data); } - function exposedSetVerifier(address target) external { - callbackVerifiers[target] = true; + function exposedSetCallbackVerifier(address target) external { + _setCallbackVerifier(target); } - function exposedRemoveVerifier(address target) external {} + function exposedRemoveCallbackVerifier(address target) external { + _removeCallbackVerifier(target); + } } contract CallbackVerificationDispatcherTest is Constants { CallbackVerificationDispatcherExposed dispatcherExposed; - event VerifierSet(address indexed executor); - event VerifierRemoved(address indexed executor); + event CallbackVerifierSet(address indexed callbackVerifier); + event CallbackVerifierRemoved(address indexed callbackVerifier); function setUp() public { uint256 forkBlock = 20673900; @@ -48,6 +50,37 @@ contract CallbackVerificationDispatcherTest is Constants { 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, @@ -56,7 +89,7 @@ contract CallbackVerificationDispatcherTest is Constants { // Thus, this test case designed from scratch using previously-deployed // Maverick callback verifier. Looking at the code, we can easily design // passing calldata. - dispatcherExposed.exposedSetVerifier( + dispatcherExposed.exposedSetCallbackVerifier( address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8) ); bytes memory data = @@ -86,7 +119,7 @@ contract CallbackVerificationDispatcherTest is Constants { // Thus, this test case designed from scratch using previously-deployed // Maverick callback verifier. Looking at the code, we can easily design // passing calldata. - dispatcherExposed.exposedSetVerifier( + dispatcherExposed.exposedSetCallbackVerifier( address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8) ); @@ -113,7 +146,7 @@ contract CallbackVerificationDispatcherTest is Constants { function testCallVerifierBadSelector() public { // A bad selector is provided to an approved executor - causing the call // itself to fail. Make sure this actually reverts. - dispatcherExposed.exposedSetVerifier( + dispatcherExposed.exposedSetCallbackVerifier( address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8) ); vm.startPrank(address(0xD0b2F5018B5D22759724af6d4281AC0B13266360)); @@ -127,7 +160,7 @@ contract CallbackVerificationDispatcherTest is Constants { function testCallVerifierParseRevertMessage() public { // Verification should fail because caller is not a Maverick pool // Check that we correctly parse the revert message - dispatcherExposed.exposedSetVerifier( + dispatcherExposed.exposedSetCallbackVerifier( address(0x2C960bD1CFE09A26105ad3C351bEa0a3fAD0F8e8) ); bytes memory data = diff --git a/foundry/test/TychoRouter.t.sol b/foundry/test/TychoRouter.t.sol index d5019bc..c6f0386 100644 --- a/foundry/test/TychoRouter.t.sol +++ b/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 From ee5040043b54f24d5dd6c9a9879ff260474c3c18 Mon Sep 17 00:00:00 2001 From: TAMARA LIPOWSKI Date: Mon, 27 Jan 2025 10:08:30 -0500 Subject: [PATCH 3/3] docs: No repetition in testCallVerifierNoSelector --- foundry/test/CallbackVerificationDispatcher.t.sol | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/foundry/test/CallbackVerificationDispatcher.t.sol b/foundry/test/CallbackVerificationDispatcher.t.sol index 308fc8f..8480a96 100644 --- a/foundry/test/CallbackVerificationDispatcher.t.sol +++ b/foundry/test/CallbackVerificationDispatcher.t.sol @@ -112,13 +112,9 @@ contract CallbackVerificationDispatcherTest is Constants { } function testCallVerifierNoSelector() 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. + // 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) );