Liquidity.Party/tycho-execution
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor: Move code check to CallbackVerificationDispatcher

Browse Source 
[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
This commit is contained in:
TAMARA LIPOWSKI
2025-01-24 17:15:12 -05:00
parent ad0748e9c3
commit 8ef061fd75
4 changed files with 79 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

51
foundry/test/CallbackVerificationDispatcher.t.sol
View File

@@ -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 =

33
foundry/test/TychoRouter.t.sol
View File

@@ -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