Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restrict increase, decrease, and burn and v4 initialize pool call #416

Merged
merged 21 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions contracts/base/Dispatcher.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ 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';
import {Actions} from '@uniswap/v4-periphery/src/libraries/Actions.sol';
import {PoolInitializer} from '@uniswap/v4-periphery/src/base/PoolInitializer.sol';

/// @title Decodes and Executes Commands
/// @notice Called by the UniversalRouter contract to efficiently decode and execute a singular command
Expand All @@ -26,6 +28,7 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V4SwapRout
error BalanceTooLow();
error InvalidAction(bytes4 action);
error NotAuthorizedForToken(uint256 tokenId);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just moved to the v3 migrator module

error OnlyMintAllowed();

/// @notice Executes encoded commands along with provided inputs.
/// @param commands A set of concatenated commands, each 1 byte in length
Expand Down Expand Up @@ -254,7 +257,6 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V4SwapRout
if (!isValidAction(selector)) {
revert InvalidAction(selector);
}

uint256 tokenId;
assembly {
// tokenId is always the first parameter in the valid actions
Expand All @@ -269,9 +271,40 @@ abstract contract Dispatcher is Payments, V2SwapRouter, V3SwapRouter, V4SwapRout
}

(success, output) = address(V3_POSITION_MANAGER).call(inputs);
} else if (command == Commands.V4_INITIALIZE_POOL) {
bytes4 selector;
assembly {
selector := calldataload(inputs.offset)
}
if (selector != PoolInitializer.initializePool.selector) {
revert InvalidAction(selector);
}
(success, output) = address(V4_POSITION_MANAGER).call(inputs);
hensha256 marked this conversation as resolved.
Show resolved Hide resolved
} else if (command == Commands.V4_POSITION_CALL) {
// should only call modifyLiquidities() to mint
// do not permit or approve this contract over a v4 position or someone could use this command to decrease, burn, or transfer your position
bytes4 selector;
assembly {
selector := calldataload(inputs.offset)
}
if (selector != V4_POSITION_MANAGER.modifyLiquidities.selector) {
revert InvalidAction(selector);
}

bytes calldata slice = inputs[4:];
(bytes calldata actions,) = slice.toBytes(0).decodeActionsRouterParams();
Copy link
Collaborator

@hensha256 hensha256 Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh you never use the parameters its decoding... i feel like it would be possible instead (and more efficient) to try this...
bytes calldata actions = slice.toBytes(0).toBytes(0)


uint256 numActions = actions.length;

for (uint256 actionIndex = 0; actionIndex < numActions; actionIndex++) {
uint256 action = uint8(actions[actionIndex]);

if (
action == Actions.INCREASE_LIQUIDITY || action == Actions.DECREASE_LIQUIDITY
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should all be in a helper function thats called like checkV4PositionManagerCall inside V3ToV4Migrator, and then this contract can stay way simpler. And all this logic can just go into there. So here its just

} else if (command == Commands.V4_POSITION_CALL) {
    checkV4PositionManagerCall()
    (success, output) = address(V4_POSITION_MANAGER).call{value: address(this).balance}(inputs);
}

and i'd argue we shouldve done the same for v3 but that can be separate PR to refactor

|| action == Actions.BURN_POSITION
) {
revert OnlyMintAllowed();
}
}
(success, output) = address(V4_POSITION_MANAGER).call{value: address(this).balance}(inputs);
} else {
// placeholder area for commands 0x13-0x20
Expand Down
5 changes: 3 additions & 2 deletions contracts/libraries/Commands.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ library Commands {
uint256 constant V4_SWAP = 0x10;
uint256 constant V3_POSITION_MANAGER_PERMIT = 0x11;
uint256 constant V3_POSITION_MANAGER_CALL = 0x12;
uint256 constant V4_POSITION_CALL = 0x13;
// COMMAND_PLACEHOLDER = 0x14 -> 0x20
uint256 constant V4_INITIALIZE_POOL = 0x13;
uint256 constant V4_POSITION_CALL = 0x14;
// COMMAND_PLACEHOLDER = 0x15 -> 0x20

// Command Types where 0x21<=value<=0x3f
uint256 constant EXECUTE_SUB_PLAN = 0x21;
Expand Down
2 changes: 1 addition & 1 deletion test/integration-tests/UniswapV2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe('Uniswap V2 Tests:', () => {

beforeEach(async () => {
// cancel the permit on DAI
await permit2.approve(DAI.address, ADDRESS_ZERO, 0, 0)
await permit2.approve(DAI.address, router.address, 0, 0)
})

it('V2 exactIn, permiting the exact amount', async () => {
Expand Down
Loading
Loading