From c85c353e344a1fec4a2fbcd5b460b37f2edfc91e Mon Sep 17 00:00:00 2001 From: Diana Carvalho Date: Fri, 31 Jan 2025 12:17:24 +0000 Subject: [PATCH] fix: Fix token index order in strategy encoding. The contract relies on this order!! Remove TODOs and comments --- don't change below this line --- ENG-4081 Took 1 hour 12 minutes Took 12 seconds --- foundry/src/TychoRouter.sol | 7 +--- foundry/test/TychoRouter.t.sol | 13 ++----- foundry/test/TychoRouterTestSetup.sol | 5 +-- .../evm/strategy_encoder/strategy_encoders.rs | 35 ++++++++++++------- 4 files changed, 26 insertions(+), 34 deletions(-) diff --git a/foundry/src/TychoRouter.sol b/foundry/src/TychoRouter.sol index 497c858..d125212 100644 --- a/foundry/src/TychoRouter.sol +++ b/foundry/src/TychoRouter.sol @@ -14,7 +14,6 @@ import "@uniswap/v3-updated/CallbackValidationV2.sol"; import "./ExecutionDispatcher.sol"; import "./CallbackVerificationDispatcher.sol"; import {LibSwap} from "../lib/LibSwap.sol"; -import "forge-std/console.sol"; error TychoRouter__WithdrawalFailed(); error TychoRouter__AddressZero(); @@ -198,7 +197,6 @@ contract TychoRouter is uint256[] memory remainingAmounts = new uint256[](nTokens); uint256[] memory amounts = new uint256[](nTokens); - console.logUint(amountIn); amounts[0] = amountIn; remainingAmounts[0] = amountIn; @@ -210,13 +208,10 @@ contract TychoRouter is currentAmountIn = split > 0 ? (amounts[tokenInIndex] * split) / 0xffffff : remainingAmounts[tokenInIndex]; - console.logUint(split); - console.logUint(tokenInIndex); // This gives 1, I guess it should be 0 - console.logUint(currentAmountIn); + currentAmountOut = _callExecutor( swapData.executor(), swapData.executorSelector(), - // TODO 0 is being passed here which makes it fail currentAmountIn, swapData.protocolData() ); diff --git a/foundry/test/TychoRouter.t.sol b/foundry/test/TychoRouter.t.sol index daf8957..a8073ab 100644 --- a/foundry/test/TychoRouter.t.sol +++ b/foundry/test/TychoRouter.t.sol @@ -682,25 +682,16 @@ contract TychoRouterTest is TychoRouterTestSetup { // Approve permit2 vm.startPrank(ALICE); - IERC20(WETH_ADDR).approve( - address(0x000000000022D473030F116dDEE9F6B43aC78BA3), - type(uint256).max - ); - + IERC20(WETH_ADDR).approve(address(permit2Address), type(uint256).max); // Encoded solution generated using `test_split_swap_strategy_encoder` but // manually replacing the executor address `5c2f5a71f67c01775180adc06909288b4c329308` // with the one in this test `5615deb798bb3e4dfa0139dfa1b3d433cc23b72f` (bool success,) = tychoRouterAddr.call( - hex"4860f9ed0000000000000000000000000000000000000000000000000de0b6b3a7640000000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000006b175474e89094c44da98b954eedeac495271d0f0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000cd09f75e2bf2a4d11f3ab23f1389fcc1621c0cc2000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000000000000000000000000000000de0b6b3a76400000000000000000000000000000000000000000000000000000000000067c3df5a00000000000000000000000000000000000000000000000000000000000000000000000000000000000000003ede3eca2a72b3aecc820e955b36f38437d0139500000000000000000000000000000000000000000000000000000000679c5962000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002800000000000000000000000000000000000000000000000000000000000000041d3d0a64c002bdbedfa0dd859cba103fed3337b0bac6ec26ed22f83475426b83a58fd79adfeefea58c5f2e52e915e20a57220c8f32578d942ab5dc15fca3f47241c00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005c005a01000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72fbd0625abc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a478c2975ab1ea89e8196811f51a7b7ade33eb113ede3eca2a72b3aecc820e955b36f38437d013950000000000" + hex"4860f9ed0000000000000000000000000000000000000000000000000de0b6b3a7640000000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000006b175474e89094c44da98b954eedeac495271d0f0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002000000000000000000000000cd09f75e2bf2a4d11f3ab23f1389fcc1621c0cc2000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc20000000000000000000000000000000000000000000000000de0b6b3a76400000000000000000000000000000000000000000000000000000000000067c43ba900000000000000000000000000000000000000000000000000000000000000000000000000000000000000003ede3eca2a72b3aecc820e955b36f38437d0139500000000000000000000000000000000000000000000000000000000679cb5b10000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000028000000000000000000000000000000000000000000000000000000000000000415bfd02ffd61c11192d1b54d76e0af125afbb32568aad37ec35f918bd5fb304cd314954213ed77c0d071301ddc45243ad57e86fe18f2905b682acc4f1a43ad8dc1c00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005c005a00010000005615deb798bb3e4dfa0139dfa1b3d433cc23b72fbd0625abc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2a478c2975ab1ea89e8196811f51a7b7ade33eb113ede3eca2a72b3aecc820e955b36f38437d013950000000000" ); - // TODO why does this only work when Alice is the caller? I tried to say the - // sender is address(this) but that doesn't work... vm.stopPrank(); - console.logAddress((address(usv2Executor))); - console.logAddress(address(this)); - console.logUint(block.timestamp); uint256 balancerAfter = IERC20(DAI_ADDR).balanceOf(ALICE); assertTrue(success, "Call Failed"); diff --git a/foundry/test/TychoRouterTestSetup.sol b/foundry/test/TychoRouterTestSetup.sol index 96b7107..839e3ee 100644 --- a/foundry/test/TychoRouterTestSetup.sol +++ b/foundry/test/TychoRouterTestSetup.sol @@ -39,10 +39,7 @@ contract TychoRouterTestSetup is Test, Constants { MockERC20[] tokens; function setUp() public { - // TODO I changed the forked block to match the signature - // of the integration test and now all the other tests fail - // fix this when the integration test passes. - uint256 forkBlock = 21742149; + uint256 forkBlock = 21000000; vm.createSelectFork(vm.rpcUrl("mainnet"), forkBlock); vm.startPrank(ADMIN); diff --git a/src/encoding/evm/strategy_encoder/strategy_encoders.rs b/src/encoding/evm/strategy_encoder/strategy_encoders.rs index c9dd737..7970a09 100644 --- a/src/encoding/evm/strategy_encoder/strategy_encoders.rs +++ b/src/encoding/evm/strategy_encoder/strategy_encoders.rs @@ -1,6 +1,6 @@ -use std::{cmp::max, str::FromStr}; +use std::{cmp::max, collections::HashSet, str::FromStr}; -use alloy_primitives::{aliases::U24, map::HashSet, FixedBytes, U256, U8}; +use alloy_primitives::{aliases::U24, FixedBytes, U256, U8}; use alloy_sol_types::SolValue; use num_bigint::BigUint; use tycho_core::{keccak256, models::Chain, Bytes}; @@ -88,18 +88,30 @@ impl StrategyEncoder for SplitSwapStrategyEncoder { min_amount_out = max(user_specified_min_amount, expected_amount_with_slippage); } } + // The tokens array is composed of the given token, the checked token and all the + // intermediary tokens in between. The contract expects the tokens to be in this order. + let solution_tokens: HashSet = + vec![solution.given_token.clone(), solution.checked_token.clone()] + .into_iter() + .collect(); - let mut tokens: Vec = solution + let intermediary_tokens: HashSet = solution .swaps .iter() .flat_map(|swap| vec![swap.token_in.clone(), swap.token_out.clone()]) - .collect::>() - .into_iter() .collect(); - + let mut intermediary_tokens: Vec = intermediary_tokens + .difference(&solution_tokens) + .cloned() + .collect(); // this is only to make the test deterministic (same index for the same token for different // runs) - tokens.sort(); + intermediary_tokens.sort(); + + let mut tokens = Vec::with_capacity(2 + intermediary_tokens.len()); + tokens.push(solution.given_token.clone()); + tokens.extend(intermediary_tokens); + tokens.push(solution.checked_token.clone()); let mut swaps = vec![]; for swap in solution.swaps.iter() { @@ -124,7 +136,6 @@ impl StrategyEncoder for SplitSwapStrategyEncoder { U8::from( tokens .iter() - // TODO Something is wrong with our token in and out indices .position(|t| *t == swap.token_in) .ok_or_else(|| { EncodingError::InvalidInput( @@ -135,7 +146,6 @@ impl StrategyEncoder for SplitSwapStrategyEncoder { U8::from( tokens .iter() - // TODO Something is wrong with our token in and out indices .position(|t| *t == swap.token_out) .ok_or_else(|| { EncodingError::InvalidInput( @@ -325,7 +335,7 @@ mod tests { checked_token: dai, expected_amount: BigUint::from_str("3_000_000000000000000000").unwrap(), check_amount: None, - sender: Bytes::from_str("0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496").unwrap(), + sender: Bytes::from_str("0xcd09f75E2BF2A4d11F3AB23f1389FcC1621c0cc2").unwrap(), receiver: Bytes::from_str("0xcd09f75E2BF2A4d11F3AB23f1389FcC1621c0cc2").unwrap(), swaps: vec![swap], ..Default::default() @@ -372,8 +382,8 @@ mod tests { // ple encoded swaps "005a", // Swap header - "01", // token in index - "00", // token out index + "00", // token in index + "01", // token out index "000000", // split // Swap data "5c2f5a71f67c01775180adc06909288b4c329308", // executor address @@ -387,7 +397,6 @@ mod tests { )); let hex_calldata = encode(&calldata); - println!("{}", hex_calldata); assert_eq!(hex_calldata[..520], expected_input); assert_eq!(hex_calldata[1288..], expected_swaps); }