From 79e04035311919112f6dd2815e880017661cec59 Mon Sep 17 00:00:00 2001 From: dianakocsis Date: Wed, 16 Oct 2024 18:01:35 -0400 Subject: [PATCH] v3 refactor and pull latest periphery --- contracts/base/Dispatcher.sol | 34 +------------- contracts/modules/Payments.sol | 2 +- contracts/modules/V3ToV4Migrator.sol | 45 ++++++++++++++++++- lib/v4-periphery | 2 +- .../UniversalRouter.gas.test.ts.snap | 2 +- .../V3ToV4Migration.gas.test.ts.snap | 12 ++--- 6 files changed, 54 insertions(+), 43 deletions(-) diff --git a/contracts/base/Dispatcher.sol b/contracts/base/Dispatcher.sol index 37730d60..3ec1272a 100644 --- a/contracts/base/Dispatcher.sol +++ b/contracts/base/Dispatcher.sol @@ -12,7 +12,6 @@ import {Commands} from '../libraries/Commands.sol'; import {Lock} from './Lock.sol'; import {ERC20} from 'solmate/src/tokens/ERC20.sol'; import {IAllowanceTransfer} from 'permit2/src/interfaces/IAllowanceTransfer.sol'; -import {IERC721Permit} from '@uniswap/v3-periphery/contracts/interfaces/IERC721Permit.sol'; import {ActionConstants} from '@uniswap/v4-periphery/src/libraries/ActionConstants.sol'; import {CalldataDecoder} from '@uniswap/v4-periphery/src/libraries/CalldataDecoder.sol'; @@ -24,8 +23,6 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V4SwapRout error InvalidCommandType(uint256 commandType); error BalanceTooLow(); - error InvalidAction(bytes4 action); - error NotAuthorizedForToken(uint256 tokenId); /// @notice Executes encoded commands along with provided inputs. /// @param commands A set of concatenated commands, each 1 byte in length @@ -237,37 +234,10 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V4SwapRout _executeActions(inputs); // This contract MUST be approved to spend the token since its going to be doing the call on the position manager } else if (command == Commands.V3_POSITION_MANAGER_PERMIT) { - bytes4 selector; - assembly { - selector := calldataload(inputs.offset) - } - if (selector != IERC721Permit.permit.selector) { - revert InvalidAction(selector); - } - + _checkV3PermitCall(inputs); (success, output) = address(V3_POSITION_MANAGER).call(inputs); } else if (command == Commands.V3_POSITION_MANAGER_CALL) { - bytes4 selector; - assembly { - selector := calldataload(inputs.offset) - } - if (!isValidAction(selector)) { - revert InvalidAction(selector); - } - - uint256 tokenId; - assembly { - // tokenId is always the first parameter in the valid actions - tokenId := calldataload(add(inputs.offset, 0x04)) - } - // If any other address that is not the owner wants to call this function, it also needs to be approved (in addition to this contract) - // This can be done in 2 ways: - // 1. This contract is permitted for the specific token and the caller is approved for ALL of the owner's tokens - // 2. This contract is permitted for ALL of the owner's tokens and the caller is permitted for the specific token - if (!isAuthorizedForToken(msgSender(), tokenId)) { - revert NotAuthorizedForToken(tokenId); - } - + _checkV3PositionManagerCall(inputs, msgSender()); (success, output) = address(V3_POSITION_MANAGER).call(inputs); } else if (command == Commands.V4_POSITION_CALL) { // should only call modifyLiquidities() to mint diff --git a/contracts/modules/Payments.sol b/contracts/modules/Payments.sol index e0630203..022e7cab 100644 --- a/contracts/modules/Payments.sol +++ b/contracts/modules/Payments.sol @@ -3,7 +3,7 @@ pragma solidity ^0.8.24; import {Constants} from '../libraries/Constants.sol'; import {ActionConstants} from '@uniswap/v4-periphery/src/libraries/ActionConstants.sol'; -import {BipsLibrary} from '@uniswap/v4-core/src/libraries/BipsLibrary.sol'; +import {BipsLibrary} from '@uniswap/v4-periphery/src/libraries/BipsLibrary.sol'; import {PaymentsImmutables} from '../modules/PaymentsImmutables.sol'; import {SafeTransferLib} from 'solmate/src/utils/SafeTransferLib.sol'; import {ERC20} from 'solmate/src/tokens/ERC20.sol'; diff --git a/contracts/modules/V3ToV4Migrator.sol b/contracts/modules/V3ToV4Migrator.sol index 27dff7f1..589671f6 100644 --- a/contracts/modules/V3ToV4Migrator.sol +++ b/contracts/modules/V3ToV4Migrator.sol @@ -3,21 +3,62 @@ pragma solidity ^0.8.24; import {MigratorImmutables} from '../modules/MigratorImmutables.sol'; import {INonfungiblePositionManager} from '@uniswap/v3-periphery/contracts/interfaces/INonfungiblePositionManager.sol'; +import {IERC721Permit} from '@uniswap/v3-periphery/contracts/interfaces/IERC721Permit.sol'; /// @title V3 to V4 Migrator /// @notice A contract that migrates liquidity from Uniswap V3 to V4 abstract contract V3ToV4Migrator is MigratorImmutables { + error InvalidAction(bytes4 action); + error NotAuthorizedForToken(uint256 tokenId); + /// @dev validate if an action is decreaseLiquidity, collect, or burn - function isValidAction(bytes4 selector) internal pure returns (bool) { + function _isValidAction(bytes4 selector) private pure returns (bool) { return selector == INonfungiblePositionManager.decreaseLiquidity.selector || selector == INonfungiblePositionManager.collect.selector || selector == INonfungiblePositionManager.burn.selector; } /// @dev the caller is authorized for the token if its the owner, spender, or operator - function isAuthorizedForToken(address caller, uint256 tokenId) internal view returns (bool) { + function _isAuthorizedForToken(address caller, uint256 tokenId) private view returns (bool) { address owner = V3_POSITION_MANAGER.ownerOf(tokenId); return caller == owner || V3_POSITION_MANAGER.getApproved(tokenId) == caller || V3_POSITION_MANAGER.isApprovedForAll(owner, caller); } + + /// @dev check that a call is to the ERC721 permit function + function _checkV3PermitCall(bytes calldata inputs) internal pure { + bytes4 selector; + assembly { + selector := calldataload(inputs.offset) + } + + if (selector != IERC721Permit.permit.selector) { + revert InvalidAction(selector); + } + } + + /// @dev check that the v3 position manager call is a safe call + function _checkV3PositionManagerCall(bytes calldata inputs, address caller) internal view { + bytes4 selector; + assembly { + selector := calldataload(inputs.offset) + } + + if (!_isValidAction(selector)) { + revert InvalidAction(selector); + } + + uint256 tokenId; + assembly { + // tokenId is always the first parameter in the valid actions + tokenId := calldataload(add(inputs.offset, 0x04)) + } + // If any other address that is not the owner wants to call this function, it also needs to be approved (in addition to this contract) + // This can be done in 2 ways: + // 1. This contract is permitted for the specific token and the caller is approved for ALL of the owner's tokens + // 2. This contract is permitted for ALL of the owner's tokens and the caller is permitted for the specific token + if (!_isAuthorizedForToken(caller, tokenId)) { + revert NotAuthorizedForToken(tokenId); + } + } } diff --git a/lib/v4-periphery b/lib/v4-periphery index 3819809f..dd76ef0f 160000 --- a/lib/v4-periphery +++ b/lib/v4-periphery @@ -1 +1 @@ -Subproject commit 3819809f8c3aa6224ece98e83abe5fbfd15a0788 +Subproject commit dd76ef0fc94bac93cd40e57b01c9395f54bb924c diff --git a/test/integration-tests/gas-tests/__snapshots__/UniversalRouter.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/UniversalRouter.gas.test.ts.snap index 535edaa9..165b5171 100644 --- a/test/integration-tests/gas-tests/__snapshots__/UniversalRouter.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/UniversalRouter.gas.test.ts.snap @@ -1,3 +1,3 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`UniversalRouter Gas Tests gas: bytecode size 1`] = `19173`; +exports[`UniversalRouter Gas Tests gas: bytecode size 1`] = `19172`; diff --git a/test/integration-tests/gas-tests/__snapshots__/V3ToV4Migration.gas.test.ts.snap b/test/integration-tests/gas-tests/__snapshots__/V3ToV4Migration.gas.test.ts.snap index d2090132..9667217f 100644 --- a/test/integration-tests/gas-tests/__snapshots__/V3ToV4Migration.gas.test.ts.snap +++ b/test/integration-tests/gas-tests/__snapshots__/V3ToV4Migration.gas.test.ts.snap @@ -3,21 +3,21 @@ exports[`V3 to V4 Migration Gas Tests V3 Commands burn gas: erc721permit + decreaseLiquidity + collect + burn 1`] = ` Object { "calldataByteLength": 1092, - "gasUsed": 256278, + "gasUsed": 256271, } `; exports[`V3 to V4 Migration Gas Tests V3 Commands collect gas: erc721permit + decreaseLiquidity + collect 1`] = ` Object { "calldataByteLength": 964, - "gasUsed": 223926, + "gasUsed": 223920, } `; exports[`V3 to V4 Migration Gas Tests V3 Commands decrease liquidity gas: erc721permit + decreaseLiquidity 1`] = ` Object { "calldataByteLength": 740, - "gasUsed": 202194, + "gasUsed": 202191, } `; @@ -38,21 +38,21 @@ Object { exports[`V3 to V4 Migration Gas Tests V4 Commands increase gas: migrate and increase 1`] = ` Object { "calldataByteLength": 2980, - "gasUsed": 452916, + "gasUsed": 452909, } `; exports[`V3 to V4 Migration Gas Tests V4 Commands mint gas: migrate and mint 1`] = ` Object { "calldataByteLength": 2500, - "gasUsed": 591168, + "gasUsed": 591159, } `; exports[`V3 to V4 Migration Gas Tests V4 Commands mint gas: migrate weth position into eth position with forwarding 1`] = ` Object { "calldataByteLength": 2788, - "gasUsed": 595307, + "gasUsed": 595300, } `;