Access lock #404

snreynolds · 2023-11-14T14:01:06Z

Related Issue

Which issue does this pull request resolve?
This is related to #366. While this doesn't allow ANY locker to resolve deltas on behalf of others it allows hooks to have this behavior.

Hooks that elect an ACCESS_LOCK_FLAG will be able to call functions swap, modifyPosition, donate, initialize take, settle, and mint without having to acquire a new lock, meaning the resulting deltas are applied to the original locker.

Description of changes

Add a new hook permission flag ACCESS_LOCK.
Update onlyByLocker to allow the locker or the allowed hook to call into PoolManager
Updates some hooks test to account for new flags
Adds test suite for access look hooks.
Uses transient storage to save the current hook address on donate, modifyPosition, and swap.

Test file AccessLock.t.sol overview:

It tests that a hook with the ACCESS_LOCK_FLAG can call every function gated by the onlyByLocker modifier. We call each of these functions "LockActions" in the tests.
* - Mint
* - Take
* - Swap
* - ModifyPosition
* - Donate
We then make sure that each of these actions are callable from the appropriate hook callbacks (beforeModifyPosition, beforeSwap, and beforeDonate).

hensha256

Coming together nicely!

hensha256 · 2023-11-17T14:19:14Z

src/PoolManager.sol

        address locker = Lockers.getCurrentLocker();
-        if (msg.sender != locker) revert LockedBy(locker);
+        if (msg.sender != locker) {
+            if (msg.sender != CurrentHookAddress.get() || !Hooks.shouldAccessLock(IHooks(CurrentHookAddress.get()))) {


Cache CurrentHookAddress.get()?

I get stack too deep :/ Any ideas? I left a comment above to trim but I couldn't think of anything

btw was able to fix stack too deep by doing this

https://github.com/Uniswap/v4-core/compare/access-lock...stack-too-deep?expand=1

but it adds gas :/

src/PoolManager.sol

src/libraries/Hooks.sol

src/test/AccessLockHook.sol

src/test/PoolDonateTest.sol

src/test/PoolSwapTest.sol

test/Hooks.t.sol

marktoda

Looks awesome! Some thoughts -

src/PoolManager.sol

marktoda · 2023-11-17T18:11:22Z

src/PoolManager.sol

@@ -185,6 +193,8 @@ contract PoolManager is IPoolManager, Fees, NoDelegateCall, Claims {
        IPoolManager.ModifyPositionParams memory params,
        bytes calldata hookData
    ) external override noDelegateCall onlyByLocker returns (BalanceDelta delta) {
+        CurrentHookAddress.set(address(key.hooks));


dont we also want to set current hook for initialize?

Hm I thought it wasn't necessary because you don't need a lock to call initialize, but maybe we should?

Hm this just made me realize that there is actually strange behavior (not a security concern bc I think things will revert) but if a hook calls into any of these functions and there is NO current locker then the deltas accrue to the 0 address. Maybe we want to restrict that there is at least one locker?

And so there are a few scenarios Im thinking of.. lmk which behavior makes the most sense to you:

If we don't set current hook in initialize and someone calls initialize with no lock - The hook will have to acquire a lock to call any of the functions.

If we don't set current hook in initialize and someone has a lock already but calls initialize. The hook can't operate on the outside lock.

If we do set current hook in initialize and someone calls initialize with no lock. the hook can operate on the 0 address lock meaning any deltas applied will auto revert.

If we do set current hook in initialize and someone calls initialize in their current lock. the hook can operate on the current lock.

Maybe 4 is the most desired? But we need to prevent 3 I think.. unless we're ok with that strange revert behavior

I think since we are allowing for AccessLock to be used with only beforeInitialize set we should allow for the same behavior (lock, hook operates on current lock delta) which would be 4? I agree 3 seems weird and like a DoS vector

Actually, this is a critical bug.

In the current code, a hook that has an access lock permission that has successfully set the LAST currentHook address to itself can call modifyPosition/swap/donate/mint/take successfully without settling deltas because it can call these functions OUTSIDE the lock.

I've reproduced the vulnerability with this test here.

I think there are two things we can do
a) require there must be a lock ie in onlyByLocker we can check that if msg.sender is not the current locker and if the current locker == addr(0) we revert

b) at the end of a lock clear the currentHook address.

My gut is to actually do both to be safe but I think a) alone would solve the issue.

B) by itself I think actually does not solve the bug if we were to add _setCurrentLocker in initialize. This is because we would have set the current hook OUTSIDE of a lock so we would never actually clear the current hook because that path would never be hit.

I also wonder if any of this warrants initialize needing a lock?

Really nice find sara! Definitely agree that (a) is the way to go.

And I think I agree that we should add a lock to initialize, and do the exact same currentHook flow for that one too.

If we think that hooks might want to do things in before/afterInitialize (which seems like a reasonable assumption)... it feels right that those actions might add deltas to pools and we should add locking too.

src/PoolManager.sol

src/libraries/Hooks.sol

test/Hooks.t.sol

marktoda · 2023-11-17T19:08:58Z

test/AccessLock.t.sol

+        // Should have less balance in both currencies.
+        assertLt(balanceOfAfter0, balanceOfBefore0);
+        assertLt(balanceOfAfter1, balanceOfBefore1);
+    }


what about some edge casier tests like:

doing one hook operation, then another - the first hook should no longer have access rights but the new one should

doing one hook operation, then an operation on a pool with no hook - the first hook should no longer have access rights

.. im sure we can think of more

I'll add a hook accessing a lock and applying deltas when there is no outside locker

The per-hook block is increasing in size over time and now is roughly 8 lines of code duplicated 8 times after #404 and #324. This commit attempts to refactor some of this copied logic into the Hooks library to improve readability of the core PoolManager code while also removing duplicated code This is a proof of concept, I'll wait until the above PRs are merged to include them in this model and can improve testing a lot with this new approach as well

src/PoolManager.sol

src/test/AccessLockHook.sol

zhongeric

looks good just some questions!

src/PoolManager.sol

src/libraries/Hooks.sol

src/libraries/Lockers.sol

src/test/PoolModifyPositionTest.sol

zhongeric · 2023-11-29T19:03:58Z

test/AccessLock.t.sol

+     *  - Mint
+     *  - Take
+     *  - Swap
+     *  - ModifyPosition
+     *  - Donate


why is Burn / Settle not tested? Is it because they are purely returning money to core?

Ah yeah I can add tests for those

test/AccessLock.t.sol

test/CurrentHookAddress.t.sol

test/Hooks.t.sol

src/PoolManager.sol

* forge install: erc-6909 * migrate 1155 to 6909 * rm old 6909 * forge install: erc-6909 * add event arg * rm old 6909 * forge install: erc-6909 * update test event * Add gas snaps * squash: support arbitrary calldata on test routers (#361) * Chore: update licenses (#364) * chore: update README * chore: update interface licenses * chore: update Hooks.sol license * chore: update types licenses * Migrate SwapMath tests to foundry (#363) * write SwapMath Tests * write gas snapshots tests * delete SwapMath hardhat implementation * eliminate SwapMathTest + add gas snapshots * delete js snapshots * migrate echidna test * forge fmt * test titles * remove console import --------- Co-authored-by: Job Mwitah <[email protected]> Co-authored-by: Mwitah <[email protected]> * add base hook for tests (#377) * change natspec to ILockCallback.lockAcquired (#376) * feat: update to solidity 0.8.22 (#378) - Enforce evm_version to avoid compiling push0 - Remove unchecked loops which are unchecked by default in 0.8.22 * Cache dynamic fee in slot0 (#360) * Bug: Require different currencies (#380) * Require different currencies * hardhat snapshots * Add new custom type function * remove extra paren * Replace MockERC20 with solmate's MockERC20 (#374) * rename MockERC20 -> UniMockERC20 * remove UniMockERC20 in favor of solmate/MockERC20 * update snapshots * Fixing compiler warnings (#386) * fix: add gas snaps for swaps with 1155 as input/output (#383) * Add snaps for 1155 swaps * remove lib * Add gas prefix * Delete Hardhat (#372) Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Alice <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> * feat: use standard forge directory structure (#389) * feat: standard foundry directory structure This commit moves to the standard foundry directory structure with contracts in src/ and tests in test/. * feat: remove JS stuff * fix: remove out dir * fix: workflows * fix: re-add js scripts * fix: add back js stuff * feat: yarn lock * fix: alice comments * Improve forge tests (#391) * Updating lots of tests * Fixed final test * remove console2 * mark PR comment * Fix Issue #397: Incorrect Documentation (#399) * feat: move whitepapers to docs dir to cleanup root (#393) * feat: move whitepapers to docs dir to cleanup root * fix: remove draft so links dont break * add snapshots to CI with tolerance (#401) * Implement Claims accounting as minimal balance (#379) * Add MinimalBalance * Initial commmit * Router custodies Claims, has access to priviledged burnFrom anbd tests * updategas * remove 6909 lib * yarn snapshots * Add gas snaps for swapping from claims balance * fix gas snaps by removing aux logic in router * gas * remove lib * Add transfer to minimalBalance, update tests * nit: rename * add back custom errors * move addition out of unchecked * Add transfer overflow check * Rename impl test * nit comments * comment# * Remove unused inheritance * remove comment * Remove poolClaimTest * fix interfaces * Feedback * Add address(0) and address(this) check for transfer * remove address(0) check * Remove batchBurn * Move mock claims to diff file * Add gas snaps for collect protocol fees * Add balance checks, make balances mapping private * Fix imports * fix fs perms * Remove uint256 in mapping and use Currency * feedback * Add gas snaps * fix: whitepaper link (#400) I accidentally broke the link from readme * Add solc binaries and transient storage lock library (#395) * Updated solc setup and instructions (#406) * Added contribution instructions * missing space * Updated solc config * feat: move JS scripts to subdir and add helper (#405) This commit moves our JS helper scripts into a test subdirectory and adds a helper abstract contract to more easily build the ffi commands to interop with javascript testers * Set tick spacing range as constants (#369) * Set tick spacing range as constants While tick and tick spacing both use int24 as their type, each has a different range. Tick spacing has a range of [1, 32767]. This commit updates Tick test cases to use proper tick spacing range instead that of tick. Resolves issue #371 * Restore a unit test on tick spacing liquidity This commit adds back the unit test that checks for tick spacing liquidity given the entire tick range as the input argument. This is an alternative change mentioned on the issue referred below. resolves #369 * Remove duplicate constants from test suite This commit moves MIN_TICK, MAX_TICK, MIN_TICK_SPACING, and MAX_TICK_SPACING constants from test suite constants file to TickMath library. Previous to this commit, TickMath library declared MIN_TICK and MAX_TICK constants with the same value from the test suite constants file. Removing duplicate constants from the test file and referencing them from the production file prevents future dicrepancies between production and test environments. * Remove unused import * Remove unnecessary override keywords * Update forge snapshots * Part 1: Improve forge tests (#407) * Revert messages * Common take and settle contract for tests * improving swap and take tests * add asserts for modify position * extra asserts in modify position * asserts in donate test * More deployment helpers * native set up in deployer * simplify initialize tests * few more corrections * more cleanup * remove console logs * snapshots and test fix post merge * snapshots * accidentally pushed foundry.toml * PR comments * remove forge snapshot --check (#417) * feat: update justfile with custom solc (#418) For folks who dont want to update their global env they can use `just test` or `just build` which sets solc using cli arg * Fix typos (#365) * Fix typos * Fix typo in src/libraries/Lockers.sol * Update just prep and build (#421) * udpate prep and add build * Update justfile * fix tests * remove totalsupply * remove lib * remove solmate * forge install: solmate main * Add comment * Add solmate 6909 and remove claims * feat: add variadic args to justfile (#423) allows to pass on args to forge i.e. `forge test --mt fuzz` * move up settle and remove solmate * forge install: solmate * copy erc6909 locally and use _mint and _burn * forge fmt * remove lib * forge install: solmate v6 * rmeove solmate * forge install: solmate 2001af43ae * fix gas snaps * remove unchecked without totalSupply * Lock on initialize (#424) * Lock on initialize * Rename initialize error * fix CI fuzz edge case * NoOp implementation with flag (#324) * cherrypicking * tests running and update snaps * test supoprt * format and run hardhat tests * remove irrelevant comment * test noops * add tests for noop * lint and snapshots * fix hook tests * clean * foundry toml * helper function * Revert early if pool isnt initialized * Extra tests * Tests NoOps on disallowed hooks fail * linting * snapshots * helper function for initialized pool * PR comments * PR comment test coverage * Final PR comments * comments about sentinel value * Fix masking tests * decrease calls to assume --------- Co-authored-by: Alex <[email protected]> Co-authored-by: hensha256 <[email protected]> * update testss * rm solmate * forge install: solmate main * Use solmate 6909 * fmt * cache msg sender * Explain different solc options (#429) * explain solc options * Add reference in readme * random linting issue * Access lock (#404) * Fix broken tests that were using old claims balance format * Add V46909 and reorder params * Add V46909 and Mock contract for test * Add revert tests * burnFrom internal * remove old gas snaps and add native tests * feat: add lock target (#300) * feat: add lock target * feat: store lockOriginator * fix: tests * feat: lockOriginator => lockCaller * feat: add invalid locker tests * fix: remove unused params * fix tests and snaps * forge fmt * expectEmit() all * expect emit all * Prevent ProtocolFeeController from bricking pool initialization on revert (#362) * Default protocol fees to 0 if protocolFeeController reverts * snapshots * fix comment * add another malicious contract * remove useless var assignment * make call and decode return in assembly * fix typo in var assingmenet * clean up tests * update snapshots# * check withdrawFee == 0 on tests too * add test * separate out fetchProtocolFees and checkProtocolFees * manually revert in setProtocolFees * Add success return value to fetchProtocolFEes * Add test for FeeTooLarge passing on initialzie but not on setProtocolFees * check low level call success * clean up * add comment * simplify * udpate snaps * fix compiler warnings * fix result type * fix formatting * Add more descriptive error message * Change error name again * Add comments * update gas specs * fix merge conflict t4ests * fix tests * Add tests for setProtocolFee with invalid controllers * Add missing snaps * Feedback changes * fix initialize tests * fix fmt * Fix comment * comments and revise order --------- Co-authored-by: Sara Reynolds <[email protected]> * feedback * Add IERC69009, does not compile * Revert "Add IERC69009, does not compile" This reverts commit 4e89408. * added delta overflow checks (#433) * add pool getters (#438) * add getters * update snaps * clean test * A few cleanup tasks (#437) * Fix compiler warnings * todo for mapping transient * fixing tests with fuzzing * remove console logs * Update PoolDonateTest.sol * remove amount overload --------- Co-authored-by: Sara Reynolds <[email protected]> * Remove hook fees and protocol fee on withdrawal (#432) * Remove hook fees and protocol fee on withdrawal * hook fee tests * Update AccessLockHook.sol * assume -> bound * more comments on fee grnaularity * refactor: hooks callsites (#439) * feat: hooks refactor * fix: tests * feat: noop -> shouldExecute * fix: remove shouldCall functions for helper * fix: using for in test * fix: alice comment * merge conflicts * fix fialing test * Copy solmate 6909 locally to get interface inheritance * natspec * expose getters in pool interface * Use id for mint/burn instead of currency * forge fmt * Add burnFrom no approval test * feedback * feedback * forge install: ERC-6909 main * remove lib * Add commit SHA * merge conflicts galore * forge install: solmate 4b47a19038b798b4a33d9749d25e570443520647 * fix conflict * fix libs * del * forge install: openzeppelin-contracts v4.4.2 * fix tests from conflcits and gas * forge fmt * removed unused file * fix: faling fuzz tests (#441) * Add failing fuzz test * Add bounds * added out of range checks * changed assume to bound * forge fmt * changed Position -> Liquidity on merge main * moved amount helper to utils --------- Co-authored-by: Austin Adams <[email protected]> * fix gas snap * forge test * feedback --------- Co-authored-by: jtriley.eth <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: Erin Koen <[email protected]> Co-authored-by: Emily Williams <[email protected]> Co-authored-by: Job Mwitah <[email protected]> Co-authored-by: Mwitah <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Ed Mazurek <[email protected]> Co-authored-by: marktoda <[email protected]> Co-authored-by: Alice <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Jose Carlos Montero Gomez <[email protected]> Co-authored-by: hyunchel <[email protected]> Co-authored-by: xiaolou86 <[email protected]> Co-authored-by: emma <[email protected]> Co-authored-by: Alex <[email protected]> Co-authored-by: hensha256 <[email protected]> Co-authored-by: Austin Adams <[email protected]> Co-authored-by: Austin Adams <[email protected]>

* forge install: erc-6909 * migrate 1155 to 6909 * rm old 6909 * forge install: erc-6909 * add event arg * rm old 6909 * forge install: erc-6909 * update test event * Add gas snaps * squash: support arbitrary calldata on test routers (Uniswap#361) * Chore: update licenses (Uniswap#364) * chore: update README * chore: update interface licenses * chore: update Hooks.sol license * chore: update types licenses * Migrate SwapMath tests to foundry (Uniswap#363) * write SwapMath Tests * write gas snapshots tests * delete SwapMath hardhat implementation * eliminate SwapMathTest + add gas snapshots * delete js snapshots * migrate echidna test * forge fmt * test titles * remove console import --------- Co-authored-by: Job Mwitah <[email protected]> Co-authored-by: Mwitah <[email protected]> * add base hook for tests (Uniswap#377) * change natspec to ILockCallback.lockAcquired (Uniswap#376) * feat: update to solidity 0.8.22 (Uniswap#378) - Enforce evm_version to avoid compiling push0 - Remove unchecked loops which are unchecked by default in 0.8.22 * Cache dynamic fee in slot0 (Uniswap#360) * Bug: Require different currencies (Uniswap#380) * Require different currencies * hardhat snapshots * Add new custom type function * remove extra paren * Replace MockERC20 with solmate's MockERC20 (Uniswap#374) * rename MockERC20 -> UniMockERC20 * remove UniMockERC20 in favor of solmate/MockERC20 * update snapshots * Fixing compiler warnings (Uniswap#386) * fix: add gas snaps for swaps with 1155 as input/output (Uniswap#383) * Add snaps for 1155 swaps * remove lib * Add gas prefix * Delete Hardhat (Uniswap#372) Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Alice <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> * feat: use standard forge directory structure (Uniswap#389) * feat: standard foundry directory structure This commit moves to the standard foundry directory structure with contracts in src/ and tests in test/. * feat: remove JS stuff * fix: remove out dir * fix: workflows * fix: re-add js scripts * fix: add back js stuff * feat: yarn lock * fix: alice comments * Improve forge tests (Uniswap#391) * Updating lots of tests * Fixed final test * remove console2 * mark PR comment * Fix Issue Uniswap#397: Incorrect Documentation (Uniswap#399) * feat: move whitepapers to docs dir to cleanup root (Uniswap#393) * feat: move whitepapers to docs dir to cleanup root * fix: remove draft so links dont break * add snapshots to CI with tolerance (Uniswap#401) * Implement Claims accounting as minimal balance (Uniswap#379) * Add MinimalBalance * Initial commmit * Router custodies Claims, has access to priviledged burnFrom anbd tests * updategas * remove 6909 lib * yarn snapshots * Add gas snaps for swapping from claims balance * fix gas snaps by removing aux logic in router * gas * remove lib * Add transfer to minimalBalance, update tests * nit: rename * add back custom errors * move addition out of unchecked * Add transfer overflow check * Rename impl test * nit comments * comment# * Remove unused inheritance * remove comment * Remove poolClaimTest * fix interfaces * Feedback * Add address(0) and address(this) check for transfer * remove address(0) check * Remove batchBurn * Move mock claims to diff file * Add gas snaps for collect protocol fees * Add balance checks, make balances mapping private * Fix imports * fix fs perms * Remove uint256 in mapping and use Currency * feedback * Add gas snaps * fix: whitepaper link (Uniswap#400) I accidentally broke the link from readme * Add solc binaries and transient storage lock library (Uniswap#395) * Updated solc setup and instructions (Uniswap#406) * Added contribution instructions * missing space * Updated solc config * feat: move JS scripts to subdir and add helper (Uniswap#405) This commit moves our JS helper scripts into a test subdirectory and adds a helper abstract contract to more easily build the ffi commands to interop with javascript testers * Set tick spacing range as constants (Uniswap#369) * Set tick spacing range as constants While tick and tick spacing both use int24 as their type, each has a different range. Tick spacing has a range of [1, 32767]. This commit updates Tick test cases to use proper tick spacing range instead that of tick. Resolves issue Uniswap#371 * Restore a unit test on tick spacing liquidity This commit adds back the unit test that checks for tick spacing liquidity given the entire tick range as the input argument. This is an alternative change mentioned on the issue referred below. resolves Uniswap#369 * Remove duplicate constants from test suite This commit moves MIN_TICK, MAX_TICK, MIN_TICK_SPACING, and MAX_TICK_SPACING constants from test suite constants file to TickMath library. Previous to this commit, TickMath library declared MIN_TICK and MAX_TICK constants with the same value from the test suite constants file. Removing duplicate constants from the test file and referencing them from the production file prevents future dicrepancies between production and test environments. * Remove unused import * Remove unnecessary override keywords * Update forge snapshots * Part 1: Improve forge tests (Uniswap#407) * Revert messages * Common take and settle contract for tests * improving swap and take tests * add asserts for modify position * extra asserts in modify position * asserts in donate test * More deployment helpers * native set up in deployer * simplify initialize tests * few more corrections * more cleanup * remove console logs * snapshots and test fix post merge * snapshots * accidentally pushed foundry.toml * PR comments * remove forge snapshot --check (Uniswap#417) * feat: update justfile with custom solc (Uniswap#418) For folks who dont want to update their global env they can use `just test` or `just build` which sets solc using cli arg * Fix typos (Uniswap#365) * Fix typos * Fix typo in src/libraries/Lockers.sol * Update just prep and build (Uniswap#421) * udpate prep and add build * Update justfile * fix tests * remove totalsupply * remove lib * remove solmate * forge install: solmate main * Add comment * Add solmate 6909 and remove claims * feat: add variadic args to justfile (Uniswap#423) allows to pass on args to forge i.e. `forge test --mt fuzz` * move up settle and remove solmate * forge install: solmate * copy erc6909 locally and use _mint and _burn * forge fmt * remove lib * forge install: solmate v6 * rmeove solmate * forge install: solmate 2001af43ae * fix gas snaps * remove unchecked without totalSupply * Lock on initialize (Uniswap#424) * Lock on initialize * Rename initialize error * fix CI fuzz edge case * NoOp implementation with flag (Uniswap#324) * cherrypicking * tests running and update snaps * test supoprt * format and run hardhat tests * remove irrelevant comment * test noops * add tests for noop * lint and snapshots * fix hook tests * clean * foundry toml * helper function * Revert early if pool isnt initialized * Extra tests * Tests NoOps on disallowed hooks fail * linting * snapshots * helper function for initialized pool * PR comments * PR comment test coverage * Final PR comments * comments about sentinel value * Fix masking tests * decrease calls to assume --------- Co-authored-by: Alex <[email protected]> Co-authored-by: hensha256 <[email protected]> * update testss * rm solmate * forge install: solmate main * Use solmate 6909 * fmt * cache msg sender * Explain different solc options (Uniswap#429) * explain solc options * Add reference in readme * random linting issue * Access lock (Uniswap#404) * Fix broken tests that were using old claims balance format * Add V46909 and reorder params * Add V46909 and Mock contract for test * Add revert tests * burnFrom internal * remove old gas snaps and add native tests * feat: add lock target (Uniswap#300) * feat: add lock target * feat: store lockOriginator * fix: tests * feat: lockOriginator => lockCaller * feat: add invalid locker tests * fix: remove unused params * fix tests and snaps * forge fmt * expectEmit() all * expect emit all * Prevent ProtocolFeeController from bricking pool initialization on revert (Uniswap#362) * Default protocol fees to 0 if protocolFeeController reverts * snapshots * fix comment * add another malicious contract * remove useless var assignment * make call and decode return in assembly * fix typo in var assingmenet * clean up tests * update snapshots# * check withdrawFee == 0 on tests too * add test * separate out fetchProtocolFees and checkProtocolFees * manually revert in setProtocolFees * Add success return value to fetchProtocolFEes * Add test for FeeTooLarge passing on initialzie but not on setProtocolFees * check low level call success * clean up * add comment * simplify * udpate snaps * fix compiler warnings * fix result type * fix formatting * Add more descriptive error message * Change error name again * Add comments * update gas specs * fix merge conflict t4ests * fix tests * Add tests for setProtocolFee with invalid controllers * Add missing snaps * Feedback changes * fix initialize tests * fix fmt * Fix comment * comments and revise order --------- Co-authored-by: Sara Reynolds <[email protected]> * feedback * Add IERC69009, does not compile * Revert "Add IERC69009, does not compile" This reverts commit 4e89408. * added delta overflow checks (Uniswap#433) * add pool getters (Uniswap#438) * add getters * update snaps * clean test * A few cleanup tasks (Uniswap#437) * Fix compiler warnings * todo for mapping transient * fixing tests with fuzzing * remove console logs * Update PoolDonateTest.sol * remove amount overload --------- Co-authored-by: Sara Reynolds <[email protected]> * Remove hook fees and protocol fee on withdrawal (Uniswap#432) * Remove hook fees and protocol fee on withdrawal * hook fee tests * Update AccessLockHook.sol * assume -> bound * more comments on fee grnaularity * refactor: hooks callsites (Uniswap#439) * feat: hooks refactor * fix: tests * feat: noop -> shouldExecute * fix: remove shouldCall functions for helper * fix: using for in test * fix: alice comment * merge conflicts * fix fialing test * Copy solmate 6909 locally to get interface inheritance * natspec * expose getters in pool interface * Use id for mint/burn instead of currency * forge fmt * Add burnFrom no approval test * feedback * feedback * forge install: ERC-6909 main * remove lib * Add commit SHA * merge conflicts galore * forge install: solmate 4b47a19038b798b4a33d9749d25e570443520647 * fix conflict * fix libs * del * forge install: openzeppelin-contracts v4.4.2 * fix tests from conflcits and gas * forge fmt * removed unused file * fix: faling fuzz tests (Uniswap#441) * Add failing fuzz test * Add bounds * added out of range checks * changed assume to bound * forge fmt * changed Position -> Liquidity on merge main * moved amount helper to utils --------- Co-authored-by: Austin Adams <[email protected]> * fix gas snap * forge test * feedback --------- Co-authored-by: jtriley.eth <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: Erin Koen <[email protected]> Co-authored-by: Emily Williams <[email protected]> Co-authored-by: Job Mwitah <[email protected]> Co-authored-by: Mwitah <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Ed Mazurek <[email protected]> Co-authored-by: marktoda <[email protected]> Co-authored-by: Alice <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Jose Carlos Montero Gomez <[email protected]> Co-authored-by: hyunchel <[email protected]> Co-authored-by: xiaolou86 <[email protected]> Co-authored-by: emma <[email protected]> Co-authored-by: Alex <[email protected]> Co-authored-by: hensha256 <[email protected]> Co-authored-by: Austin Adams <[email protected]> Co-authored-by: Austin Adams <[email protected]>

snreynolds force-pushed the access-lock branch 3 times, most recently from e68a5dc to 440648f Compare November 17, 2023 04:25

snreynolds marked this pull request as ready for review November 17, 2023 04:26

snreynolds requested review from marktoda and hensha256 November 17, 2023 04:30

hensha256 reviewed Nov 17, 2023

View reviewed changes

zhongeric reviewed Nov 17, 2023

View reviewed changes

src/test/PoolDonateTest.sol Show resolved Hide resolved

src/test/PoolSwapTest.sol Outdated Show resolved Hide resolved

test/Hooks.t.sol Show resolved Hide resolved

marktoda reviewed Nov 17, 2023

View reviewed changes

marktoda mentioned this pull request Nov 17, 2023

feat: hooks refactor POC #411

Closed

snreynolds added 14 commits November 20, 2023 18:11

rebase

1cc37ed

add access lock flag

2a2321e

create AccessLock tests

65069b3

add beforeModifyPosition tests for mint/swap/modify/take/donate

589026f

add beforeSwap tests

70a869e

add beforeDonate tests

e147a7c

fix fuzz boundaries

2396461

use transient storage

8b6b850

fix ci?

400ce17

fix: comments

7bda2b1

add revert test

fbda6c4

more edge case tests

11c9321

add failing test

d6864a4

fix: hooks can only call functions in the middle of a lock

d7829b7

snreynolds force-pushed the access-lock branch from b04ac50 to d7829b7 Compare November 20, 2023 23:13

fix comments

9d8f00b

snreynolds force-pushed the access-lock branch from b95d877 to 9d8f00b Compare November 20, 2023 23:20

snreynolds requested review from zhongeric and hensha256 November 20, 2023 23:22

move onlyByLocker, add comments

83afc1e

hensha256 reviewed Nov 29, 2023

View reviewed changes

src/PoolManager.sol Outdated Show resolved Hide resolved

hensha256 reviewed Nov 29, 2023

View reviewed changes

src/test/AccessLockHook.sol Outdated Show resolved Hide resolved

zhongeric reviewed Nov 29, 2023

View reviewed changes

hensha256 reviewed Nov 29, 2023

View reviewed changes

test/CurrentHookAddress.t.sol Outdated Show resolved Hide resolved

snreynolds added 3 commits November 29, 2023 17:07

update comments, use custom error, add test cases

b9645ef

add burn test

7fc2658

remove logs

e571806

snreynolds force-pushed the access-lock branch from 7b2d352 to e571806 Compare November 30, 2023 14:21

snreynolds added 2 commits November 30, 2023 11:49

add settle test

680faf7

init test

53cdf3e

zhongeric previously approved these changes Nov 30, 2023

View reviewed changes

hensha256 reviewed Nov 30, 2023

View reviewed changes

test/Hooks.t.sol Show resolved Hide resolved

add back noop test

7755acd

snreynolds dismissed zhongeric’s stale review via 7755acd November 30, 2023 22:33

hensha256 previously approved these changes Nov 30, 2023

View reviewed changes

marktoda previously approved these changes Nov 30, 2023

View reviewed changes

marktoda reviewed Nov 30, 2023

View reviewed changes

src/PoolManager.sol Outdated Show resolved Hide resolved

remove else

852ed5b

snreynolds dismissed stale reviews from marktoda and hensha256 via 852ed5b November 30, 2023 22:44

marktoda approved these changes Dec 1, 2023

View reviewed changes

Merge branch 'main' into access-lock

0aee197

snreynolds merged commit 6b8ec7c into main Dec 1, 2023
4 checks passed

snreynolds deleted the access-lock branch December 1, 2023 16:52

zhongeric pushed a commit that referenced this pull request Dec 14, 2023

Access lock (#404)

921694f

zhongeric pushed a commit that referenced this pull request Dec 14, 2023

Access lock (#404)

a319c1e

hyunchel pushed a commit to hyunchel/v4-core that referenced this pull request Feb 21, 2024

Access lock (Uniswap#404)

6b8e4ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access lock #404

Access lock #404

snreynolds commented Nov 14, 2023 •

edited

Loading

hensha256 left a comment

hensha256 Nov 17, 2023

snreynolds Nov 20, 2023

snreynolds Nov 20, 2023

marktoda left a comment

marktoda Nov 17, 2023

snreynolds Nov 20, 2023

snreynolds Nov 20, 2023 •

edited

Loading

zhongeric Nov 20, 2023

snreynolds Nov 20, 2023 •

edited

Loading

hensha256 Nov 21, 2023

marktoda Nov 17, 2023

snreynolds Nov 20, 2023

zhongeric left a comment

zhongeric Nov 29, 2023

snreynolds Nov 29, 2023

Access lock #404

Access lock #404

Conversation

snreynolds commented Nov 14, 2023 • edited Loading

Related Issue

Description of changes

hensha256 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marktoda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snreynolds Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snreynolds Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongeric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snreynolds commented Nov 14, 2023 •

edited

Loading

snreynolds Nov 20, 2023 •

edited

Loading

snreynolds Nov 20, 2023 •

edited

Loading