Make TychoRouter inherit from SafeCallback and then delegatecall to the UniswapV4 executor
Add a test for this. I had to update the block of our forked network in the tests. Because of this I had to update all the asserts in previous tests
Had to change the optimizer_runs in foundry.toml because of weird Yul errors when compiling
--- don't change below this line ---
ENG-4223 Took 1 hour 21 minutes
Took 7 seconds
Took 35 seconds
- This is for security purposes. Max uncovered a vulnerability where funds could be stolen if we don't ether the while loop (i.e. if we have empty swaps).
The issue was that we weren't indexing WETH properly since it was looking for the WETH address in tokens, when only native ETH would be in there
- Found by adding integration tests for the wrapping and unwrapping cases.
- The util function was previously expecting a value between 0 and 100, which we felt was a weird UI.
- In integration test - make sure we spent all input tokens
- Note: I think we can get the fee straight from the pool... why did we always encode this and send it from the solver? Is this bound to change sometimes?
- The funds will always go through the router
- Rename splitSwap for Swap
- Improve tests
--- don't change below this line ---
ENG-4041 Took 24 minutes
Took 23 seconds
Took 42 seconds
Add tests to Swap
Modify ExecutionDispatcher and TychoRouter to account for these changes
--- don't change below this line ---
ENG-4041 Took 57 minutes
Took 10 seconds
Changes:
- If the tokenIn is ETH, skip permit2 approval
- Make executors payable: When using delegatecall the executor inherits the execution context of whoever calls it. Our main swap function can accept ETH, it needs to be payable so by consequence the executors also need to be.
- Set uniswap v2 executor in test router
- Add tests for all possible cases of swap
- Add tests for all cases of splitSwap
- Add test functions to handle permit2 and encode swaps
--- don't change below this line ---
ENG-4041 Took 3 hours 50 minutes
Took 49 seconds
Took 14 seconds
[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 is not so relevant for security, but it would sabotage our performance if an executor was wrongly removed, so it's worth it to know every time this happens.
I was inspired to do this because, when disabling the slither check for the delegatecall when calling the swap executor, I realized it's not clear from the same contract that we have already checked for contract code existence when setting the executor. 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 delegatecall 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 checks contract validity before setting the executor in the SwapExecutionDispatcher
- Moved the deployment method into a test template for organization
- Created skeletons of dispatcher contracts
- Added all possible test cases for thoroughness
- remappings.txt is used for more elegant imports
- decided not to include all helper methods in skeleton - just main swap method. They are properly documented in the jira tasks.
- add filter-paths to slither to exclude submodules, otherwise we will get slither warnings about permit2 and open-zeppelin using different solidity versions:
```
- Version constraint ^0.8.20 is used by:
-^0.8.20 (lib/openzeppelin-contracts/contracts/access/AccessControl.sol#4)
-^0.8.20 (lib/openzeppelin-contracts/contracts/access/IAccessControl.sol#4)
-^0.8.20 (lib/openzeppelin-contracts/contracts/utils/Context.sol#4)
-^0.8.20 (lib/openzeppelin-contracts/contracts/utils/introspection/ERC165.sol#4)
-^0.8.20 (lib/openzeppelin-contracts/contracts/utils/introspection/IERC165.sol#4)
- Version constraint ^0.8.0 is used by:
-^0.8.0 (lib/permit2/src/interfaces/IAllowanceTransfer.sol#2)
-^0.8.0 (lib/permit2/src/interfaces/IEIP712.sol#2)
- Version constraint ^0.8.28 is used by:
```
