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..0e2a058 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,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:]; + } } diff --git a/foundry/src/TychoRouter.sol b/foundry/src/TychoRouter.sol index da92429..f929e9b 100644 --- a/foundry/src/TychoRouter.sol +++ b/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(); - callbackVerifiers[target] = true; - emit CallbackVerifierSet(target); + _setCallbackVerifier(target); } /** @@ -152,7 +148,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 new file mode 100644 index 0000000..8480a96 --- /dev/null +++ b/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") + ); + } +} diff --git a/foundry/test/TychoRouter.t.sol b/foundry/test/TychoRouter.t.sol index abcbdba..3d6f6d0 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