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

Access lock #404

Merged
merged 35 commits into from
Dec 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
1cc37ed
rebase
snreynolds Nov 9, 2023
2a2321e
add access lock flag
snreynolds Nov 14, 2023
65069b3
create AccessLock tests
snreynolds Nov 15, 2023
589026f
add beforeModifyPosition tests for mint/swap/modify/take/donate
snreynolds Nov 16, 2023
70a869e
add beforeSwap tests
snreynolds Nov 17, 2023
e147a7c
add beforeDonate tests
snreynolds Nov 17, 2023
2396461
fix fuzz boundaries
snreynolds Nov 17, 2023
8b6b850
use transient storage
snreynolds Nov 17, 2023
400ce17
fix ci?
snreynolds Nov 17, 2023
7bda2b1
fix: comments
snreynolds Nov 20, 2023
fbda6c4
add revert test
snreynolds Nov 20, 2023
11c9321
more edge case tests
snreynolds Nov 20, 2023
d6864a4
add failing test
snreynolds Nov 20, 2023
d7829b7
fix: hooks can only call functions in the middle of a lock
snreynolds Nov 20, 2023
9d8f00b
fix comments
snreynolds Nov 20, 2023
2a7ccf9
fix: currentHook must be updated only on the first call, and cleared
snreynolds Nov 21, 2023
2e88e5f
merge main
snreynolds Nov 22, 2023
969fb9d
use IHooks, remove 1155 ref
snreynolds Nov 22, 2023
a958a37
remove special no access lock hook
snreynolds Nov 22, 2023
f70443a
add nested lock test
snreynolds Nov 22, 2023
88d9ce1
unset hook for noOp case
snreynolds Nov 26, 2023
448b557
add initialize
snreynolds Nov 27, 2023
fa432c2
add vanilla initialize tests
snreynolds Nov 27, 2023
fb803c3
fmt error
snreynolds Nov 27, 2023
f455e64
add natspec and flatten onlyByLocker check
snreynolds Nov 27, 2023
528ce4e
snaps
snreynolds Nov 27, 2023
83afc1e
move onlyByLocker, add comments
snreynolds Nov 29, 2023
b9645ef
update comments, use custom error, add test cases
snreynolds Nov 29, 2023
7fc2658
add burn test
snreynolds Nov 30, 2023
e571806
remove logs
snreynolds Nov 30, 2023
680faf7
add settle test
snreynolds Nov 30, 2023
53cdf3e
init test
snreynolds Nov 30, 2023
7755acd
add back noop test
snreynolds Nov 30, 2023
852ed5b
remove else
snreynolds Nov 30, 2023
0aee197
Merge branch 'main' into access-lock
snreynolds Dec 1, 2023
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
Original file line number Diff line number Diff line change
@@ -1 +1 @@
190294
191802
2 changes: 1 addition & 1 deletion .forge-snapshots/cached dynamic fee, no hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
145654
147163
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 1 token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
137259
138487
2 changes: 1 addition & 1 deletion .forge-snapshots/donate gas with 2 tokens.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
186980
185645
2 changes: 1 addition & 1 deletion .forge-snapshots/erc20 collect protocol fees.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
27033
26991
2 changes: 1 addition & 1 deletion .forge-snapshots/gas overhead of no-op lock.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
14918
14940
2 changes: 1 addition & 1 deletion .forge-snapshots/initialize.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
75025
77663
2 changes: 1 addition & 1 deletion .forge-snapshots/mint with empty hook.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
319543
318152
2 changes: 1 addition & 1 deletion .forge-snapshots/mint with native token.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
199129
200911
2 changes: 1 addition & 1 deletion .forge-snapshots/mint.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
202244
200853
2 changes: 1 addition & 1 deletion .forge-snapshots/modify position with noop.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
52336
56127
2 changes: 1 addition & 1 deletion .forge-snapshots/native collect protocol fees.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
38699
38657
2 changes: 1 addition & 1 deletion .forge-snapshots/poolManager bytecode size.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25078
25255
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap with native.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
191062
196049
2 changes: 1 addition & 1 deletion .forge-snapshots/simple swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
202765
204617
Original file line number Diff line number Diff line change
@@ -1 +1 @@
121658
126645
2 changes: 1 addition & 1 deletion .forge-snapshots/swap against liquidity.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
109483
114135
2 changes: 1 addition & 1 deletion .forge-snapshots/swap burn claim for input.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
128222
133204
2 changes: 1 addition & 1 deletion .forge-snapshots/swap mint output as claim.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
212934
216787
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with dynamic fee.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
189534
191043
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with hooks.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
109462
114114
2 changes: 1 addition & 1 deletion .forge-snapshots/swap with noop.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45202
48947
2 changes: 1 addition & 1 deletion .forge-snapshots/update dynamic fee in before swap.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
196118
197626
69 changes: 57 additions & 12 deletions src/PoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,18 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
return Lockers.getLocker(i);
}

/// @notice This will revert if a function is called by any address other than the current locker OR the most recently called, pre-permissioned hook.
modifier onlyByLocker() {
_checkLocker(msg.sender, Lockers.getCurrentLocker(), Lockers.getCurrentHook());
_;
}

function _checkLocker(address caller, address locker, IHooks hook) internal pure {
if (caller == locker) return;
if (caller == address(hook) && hook.hasPermissionToAccessLock()) return;
revert LockedBy(locker, address(hook));
}

/// @inheritdoc IPoolManager
function initialize(PoolKey memory key, uint160 sqrtPriceX96, bytes calldata hookData)
external
Expand All @@ -105,6 +117,8 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
if (key.currency0 >= key.currency1) revert CurrenciesOutOfOrderOrEqual();
if (!key.hooks.isValidHookAddress(key.fee)) revert Hooks.HookAddressNotValid(address(key.hooks));

(bool set) = Lockers.setCurrentHook(key.hooks);
snreynolds marked this conversation as resolved.
Show resolved Hide resolved

if (key.hooks.shouldCallBeforeInitialize()) {
if (key.hooks.beforeInitialize(msg.sender, key, sqrtPriceX96, hookData) != IHooks.beforeInitialize.selector)
{
Expand All @@ -127,6 +141,9 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
}
}

// We only want to clear the current hook if it was set in setCurrentHook in this execution frame.
if (set) Lockers.clearCurrentHook();

// On intitalize we emit the key's fee, which tells us all fee settings a pool can have: either a static swap fee or dynamic swap fee and if the hook has enabled swap or withdraw fees.
emit Initialize(id, key.currency0, key.currency1, key.fee, key.tickSpacing, key.hooks);
}
Expand Down Expand Up @@ -174,26 +191,27 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
if (pools[id].isNotInitialized()) revert PoolNotInitialized();
}

modifier onlyByLocker() {
address locker = Lockers.getCurrentLocker();
if (msg.sender != locker) revert LockedBy(locker);
_;
}

/// @inheritdoc IPoolManager
function modifyPosition(
PoolKey memory key,
IPoolManager.ModifyPositionParams memory params,
bytes calldata hookData
) external override noDelegateCall onlyByLocker returns (BalanceDelta delta) {
(bool set) = Lockers.setCurrentHook(key.hooks);

PoolId id = key.toId();
_checkPoolInitialized(id);

snreynolds marked this conversation as resolved.
Show resolved Hide resolved
if (key.hooks.shouldCallBeforeModifyPosition()) {
bytes4 selector = key.hooks.beforeModifyPosition(msg.sender, key, params, hookData);
// Sentinel return value used to signify that a NoOp occurred.
if (key.hooks.isValidNoOpCall(selector)) return BalanceDeltaLibrary.MAXIMUM_DELTA;
else if (selector != IHooks.beforeModifyPosition.selector) revert Hooks.InvalidHookResponse();
if (key.hooks.isValidNoOpCall(selector)) {
// We only want to clear the current hook if it was set in setCurrentHook in this execution frame.
if (set) Lockers.clearCurrentHook();
return BalanceDeltaLibrary.MAXIMUM_DELTA;
} else if (selector != IHooks.beforeModifyPosition.selector) {
revert Hooks.InvalidHookResponse();
}
}

Pool.FeeAmounts memory feeAmounts;
Expand Down Expand Up @@ -233,6 +251,9 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
}
}

// We only want to clear the current hook if it was set in setCurrentHook in this execution frame.
if (set) Lockers.clearCurrentHook();

emit ModifyPosition(id, msg.sender, params.tickLower, params.tickUpper, params.liquidityDelta);
}

Expand All @@ -244,14 +265,21 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
onlyByLocker
returns (BalanceDelta delta)
{
(bool set) = Lockers.setCurrentHook(key.hooks);

PoolId id = key.toId();
_checkPoolInitialized(id);

if (key.hooks.shouldCallBeforeSwap()) {
bytes4 selector = key.hooks.beforeSwap(msg.sender, key, params, hookData);
// Sentinel return value used to signify that a NoOp occurred.
if (key.hooks.isValidNoOpCall(selector)) return BalanceDeltaLibrary.MAXIMUM_DELTA;
else if (selector != IHooks.beforeSwap.selector) revert Hooks.InvalidHookResponse();
if (key.hooks.isValidNoOpCall(selector)) {
// We only want to clear the current hook if it was set in setCurrentHook in this execution frame.
if (set) Lockers.clearCurrentHook();
return BalanceDeltaLibrary.MAXIMUM_DELTA;
} else if (selector != IHooks.beforeSwap.selector) {
revert Hooks.InvalidHookResponse();
}
}

uint256 feeForProtocol;
Expand Down Expand Up @@ -285,6 +313,9 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
}
}

// We only want to clear the current hook if it was set in setCurrentHook in this execution frame.
if (set) Lockers.clearCurrentHook();

emit Swap(
id, msg.sender, delta.amount0(), delta.amount1(), state.sqrtPriceX96, state.liquidity, state.tick, swapFee
);
Expand All @@ -298,14 +329,21 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
onlyByLocker
returns (BalanceDelta delta)
{
(bool set) = Lockers.setCurrentHook(key.hooks);

PoolId id = key.toId();
_checkPoolInitialized(id);

if (key.hooks.shouldCallBeforeDonate()) {
bytes4 selector = key.hooks.beforeDonate(msg.sender, key, amount0, amount1, hookData);
// Sentinel return value used to signify that a NoOp occurred.
if (key.hooks.isValidNoOpCall(selector)) return BalanceDeltaLibrary.MAXIMUM_DELTA;
else if (selector != IHooks.beforeDonate.selector) revert Hooks.InvalidHookResponse();
if (key.hooks.isValidNoOpCall(selector)) {
// We only want to clear the current hook if it was set in setCurrentHook in this execution frame.
if (set) Lockers.clearCurrentHook();
snreynolds marked this conversation as resolved.
Show resolved Hide resolved
return BalanceDeltaLibrary.MAXIMUM_DELTA;
} else if (selector != IHooks.beforeDonate.selector) {
revert Hooks.InvalidHookResponse();
}
}

delta = pools[id].donate(amount0, amount1);
Expand All @@ -317,6 +355,9 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
revert Hooks.InvalidHookResponse();
}
}

// We only want to clear the current hook if it was set in setCurrentHook in this execution frame.
if (set) Lockers.clearCurrentHook();
}

/// @inheritdoc IPoolManager
Expand Down Expand Up @@ -400,6 +441,10 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
return Lockers.nonzeroDeltaCount();
}

function getCurrentHook() external view returns (IHooks) {
return Lockers.getCurrentHook();
}

/// @notice receive native tokens for native pools
receive() external payable {}
}
6 changes: 5 additions & 1 deletion src/interfaces/IPoolManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ interface IPoolManager is IFees, IClaims {

/// @notice Thrown when a function is called by an address that is not the current locker
/// @param locker The current locker
error LockedBy(address locker);
/// @param currentHook The most recently called hook
error LockedBy(address locker, address currentHook);

/// @notice The ERC1155 being deposited is not the Uniswap ERC1155
error NotPoolManagerToken();
Expand Down Expand Up @@ -123,6 +124,9 @@ interface IPoolManager is IFees, IClaims {
/// @notice Returns the length of the lockers array, which is the number of locks open on the PoolManager.
function getLockLength() external view returns (uint256 _length);

/// @notice Returns the most recently called hook.
function getCurrentHook() external view returns (IHooks _currentHook);

/// @notice Returns the number of nonzero deltas open on the PoolManager that must be zerod by the close of the initial lock.
function getLockNonzeroDeltaCount() external view returns (uint256 _nonzeroDeltaCount);

Expand Down
32 changes: 20 additions & 12 deletions src/libraries/Hooks.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ library Hooks {
uint256 internal constant BEFORE_DONATE_FLAG = 1 << 153;
uint256 internal constant AFTER_DONATE_FLAG = 1 << 152;
uint256 internal constant NO_OP_FLAG = 1 << 151;
uint256 internal constant ACCESS_LOCK_FLAG = 1 << 150;

bytes4 public constant NO_OP_SELECTOR = bytes4(keccak256(abi.encodePacked("NoOp")));

struct Calls {
struct Permissions {
snreynolds marked this conversation as resolved.
Show resolved Hide resolved
bool beforeInitialize;
bool afterInitialize;
bool beforeModifyPosition;
Expand All @@ -33,6 +34,7 @@ library Hooks {
bool beforeDonate;
bool afterDonate;
bool noOp;
bool accessLock;
}

/// @notice Thrown if the address will not lead to the specified hook calls being called
Expand All @@ -44,17 +46,19 @@ library Hooks {

/// @notice Utility function intended to be used in hook constructors to ensure
/// the deployed hooks address causes the intended hooks to be called
/// @param calls The hooks that are intended to be called
/// @dev calls param is memory as the function will be called from constructors
function validateHookAddress(IHooks self, Calls memory calls) internal pure {
/// @param permissions The hooks that are intended to be called
/// @dev permissions param is memory as the function will be called from constructors
function validateHookPermissions(IHooks self, Permissions memory permissions) internal pure {
if (
calls.beforeInitialize != shouldCallBeforeInitialize(self)
|| calls.afterInitialize != shouldCallAfterInitialize(self)
|| calls.beforeModifyPosition != shouldCallBeforeModifyPosition(self)
|| calls.afterModifyPosition != shouldCallAfterModifyPosition(self)
|| calls.beforeSwap != shouldCallBeforeSwap(self) || calls.afterSwap != shouldCallAfterSwap(self)
|| calls.beforeDonate != shouldCallBeforeDonate(self) || calls.afterDonate != shouldCallAfterDonate(self)
|| calls.noOp != hasPermissionToNoOp(self)
permissions.beforeInitialize != shouldCallBeforeInitialize(self)
|| permissions.afterInitialize != shouldCallAfterInitialize(self)
|| permissions.beforeModifyPosition != shouldCallBeforeModifyPosition(self)
|| permissions.afterModifyPosition != shouldCallAfterModifyPosition(self)
|| permissions.beforeSwap != shouldCallBeforeSwap(self)
|| permissions.afterSwap != shouldCallAfterSwap(self)
|| permissions.beforeDonate != shouldCallBeforeDonate(self)
|| permissions.afterDonate != shouldCallAfterDonate(self) || permissions.noOp != hasPermissionToNoOp(self)
snreynolds marked this conversation as resolved.
Show resolved Hide resolved
|| permissions.accessLock != hasPermissionToAccessLock(self)
) {
revert HookAddressNotValid(address(self));
}
Expand All @@ -74,7 +78,7 @@ library Hooks {
return address(hook) == address(0)
? !fee.isDynamicFee() && !fee.hasHookSwapFee() && !fee.hasHookWithdrawFee()
: (
uint160(address(hook)) >= AFTER_DONATE_FLAG || fee.isDynamicFee() || fee.hasHookSwapFee()
uint160(address(hook)) >= ACCESS_LOCK_FLAG || fee.isDynamicFee() || fee.hasHookSwapFee()
|| fee.hasHookWithdrawFee()
);
}
Expand Down Expand Up @@ -111,6 +115,10 @@ library Hooks {
return uint256(uint160(address(self))) & AFTER_DONATE_FLAG != 0;
}

function hasPermissionToAccessLock(IHooks self) internal pure returns (bool) {
return uint256(uint160(address(self))) & ACCESS_LOCK_FLAG != 0;
}

function hasPermissionToNoOp(IHooks self) internal pure returns (bool) {
return uint256(uint160(address(self))) & NO_OP_FLAG != 0;
}
Expand Down
Loading