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