Middleware remove #145
Middleware remove #145
Conversation
Co-authored-by: Sara Reynolds <[email protected]>
* Clean up repo * Move safe callback
| return (BaseHook.afterRemoveLiquidity.selector, BalanceDeltaLibrary.ZERO_DELTA); | ||
| } | ||
|
|
||
| function _callAndEnsureZeroDeltas(bytes calldata data) external { |
There was a problem hiding this comment.
Can't we just not allow the hook to have the flags for AFTER_ADD_LIQUIDITY_RETURNS_DELTA_FLAG and AFTER_REMOVE_LIQUIDITY_RETURNS_DELTA_FLAG
also I can semi see a case where we'd just want to protect no reverts on remove, but not necessarily no deltas? I wonder if we could split out these two implementations and then have a third implementation that inherits them both?
There was a problem hiding this comment.
so i already have a flag check in the _ensureValidFlags override. (we don't care about add liquidity btw)
so the reason why i disallow changing deltas is because of a bug i discovered while writing this hook: if the hook improperly creates deltas but does not resolve them, the entire remove liquidity will revert because of CurrencyNotSettled. this is a revert i can not catch directly. therefore, if the hook does "unbalanced" accounting by any means, the entire hook will revert and a safe vanilla withdraw will occur.
(while typing this i realized that i also need to handle this in beforeRemoveLiquidity as well)
There was a problem hiding this comment.
ah gotcha - maybe a comment in the contract explaining this? However this problem is not really solved by the middlewear I think. IE... it doesn't really matter if we revert here or revert from within the hook, the user still won't be able to remove their liquidity...
There was a problem hiding this comment.
it actually just reverts the entire hook call. see my testFeeOnRemove hook, how the swap still goes through
| BalanceDelta delta, | ||
| bytes calldata hookData | ||
| ) external returns (bytes4, BalanceDelta) { | ||
| address(this).delegatecall{gas: GAS_LIMIT}( |
There was a problem hiding this comment.
is there a reason to delegatecall here? Is it cause we need the GAS_LIMIT enforcement for the whole remainder or the call? What if we just enforced that gasLeft() > GAS_LIMIT by some proportion so that we could internally call _callAndEnsurePrice and then do a delegatecall() with the gas limit externally.. just feels odd that we delegatecall twice?
There was a problem hiding this comment.
yes, so this is a solution that took me a while to figure out. it looks strange, but i need to perform a try catch and keep msg.sender the same. you can't try catch an internal function, and if i do .call instead of .delegatecall, msg.sender is now the middleware.
There was a problem hiding this comment.
oh well technically not a try/catch. it just doesn't require success
| uint256 countAfter = uint256(IExttload(address(manager)).exttload(slot)); | ||
| if (countAfter != countBefore) { | ||
| // purpousely revert to cause the whole hook to reset | ||
| revert HookModifiedDeltas(); |
There was a problem hiding this comment.
What exactly are you trying to prevent with this check? A hook could technically still modify deltas on the pool so long as they settle it back to the count before the call.
There was a problem hiding this comment.
it's totally ok if the hook modifies deltas as long as they settle it. as i mentioned previously, the goal is to catch reverts when a hook incorrectly settles deltas. if countAfter != countBefore, it's a heuristic that the hook messed something up, in which the entire thing reverts (see why i needed two delegatecalls?) and it becomes a vanilla hookless withdrawal.
| revert FailedImplementationCall(); | ||
| } | ||
| (uint160 priceAfter,,,) = manager.getSlot0(key.toId()); | ||
| if (priceAfter != priceBefore) { |
There was a problem hiding this comment.
this is an awesome check! Similar to my comment below I wonder if for each check we can abstract them out. And then have a Middlewear contract that inherits any combination of these checks to allow for all sorts of combinations of checks
There was a problem hiding this comment.
that would be cool, like lego blocks you can stack to create middleware checks. but is this level of modularity really necessary? i feel like there are only a few middleware designs that truly make sense. and let's say uniswap frontend decides to whitelist middleware-remove hooks for example. there's not really any incentive for new devs to make new middleware designs, as one is all we really need.
unless... the plan is to make these protections modular on the frontend. eg: middleware XYZ has these permissions but is blocked from these middleware selections. almost like an anti-virus checklist that shows up per hook. this would give so much flexibility to the hook developer!
There was a problem hiding this comment.
yeah exactly I like the lego block metaphor
rebase
* Clean up repo * Move safe callback * starting work on shared actions router * Compiling with todos * correct function * PR comments * tests for base actions router * gas test * calldata gas optimisation * linting * add todo * Optimised bytes function * Additional refactor for generalisation * comments * Added natspec * PR comments
* initial thoughts lock and batch * update safecallback with constructor * simple batch under lock * oops * misc version bump; will conflict but can resolve later * defining types and different levels of abstractions * merge in main; resolve conflicts * wip * misc fixes with main:latest * basic mint * begin moving tests to fuzz * test for slippage * burning * decrease liquidity * mint transfer burn, liquidityOf accounting * wip * refactor to use CurrencySettleTake * basic fee collection * wip * misc fix * fee collection for independent same-range parties * LiquidityPosition -> LiquidityRange * erc20 fee collection * decrease liquidity with fee collection * wip test decrease liquidity on same range * reworked fuzzers; more testing on fee claims for liquidity decreasing * forge fmt * test fixes for flipped deltas * wip * test coverage for increase liquidity cases * preliminary gas benchmarks * Position manager refactor (#2) * chore: update v4-core:latest (#105) * update core * rename lockAcquired to unlockCallback * update core; temporary path hack in remappings * update v4-core; remove remapping * wip: fix compatibility * update core; fix renaming of swap fee to lp fee * update core; fix events * update core; address liquidity salt and modify liquidity return values * fix incorrect delta accounting when modifying liquidity * fix todo, use CurrencySettleTake * remove deadcode * update core; use StateLibrary; update sqrtRatio to sqrtPrice * fix beforeSwap return signatures * forge fmt; remove commented out code * update core (wow gas savings) * update core * update core * update core; hook flags LSB * update core * update core * chore: update v4 core (#115) * Update v4-core * CurrencySettleTake -> CurrencySettler * Snapshots * compiling but very broken * replace PoolStateLibrary * update currency settle take * compiling * wip * use v4-core's forge-std * test liquidity increase * additional fixes for collection and liquidity decrease * test migration * replace old implementation with new --------- Signed-off-by: saucepoint <[email protected]> Co-authored-by: 0x57 <[email protected]> * cleanup: TODOs and imports * Position manager Consolidate (#3) * wip: consolidation * further consolidation * consolidate to single file * yay no more stack too deep * some code comments * use currency settler syntax * use v4-core's gas snapshot * use snapLastCall and isolate for posm benchmarks * Update contracts/libraries/CurrencySettleTake.sol Co-authored-by: 0x57 <[email protected]> * use v4-core's solmate its more recent * use v4-core's openzeppelin-contracts * add ERC721Permit * feedback: memory hookData * initial refactor. stack too deep * passing tests * gutted LockAndBatchCall * cleanup diff * renaming vanilla functions * sanitize * change add liq accounting (#126) * change add liq accounting * remove rand comments * fix exact fees * use closeAllDeltas * comments cleanup * additional liquidity tests (#129) * additional increase liquidity tests * edge case of using cached fees for autocompound * wip * fix autocompound bug, use custodied and unclaimed fees in the autocompound * fix tests and use BalanceDeltas (#130) * fix some assertions * use BalanceDeltas for arithmetic * cleanest code in the game??? * additional cleaning * typo lol * autocompound gas benchmarks * autocompound excess credit gas benchmark * save 600 gas, cleaner code when moving caller delta to tokensOwed --------- Co-authored-by: saucepoint <[email protected]> * create compatibility with arbitrary execute handler * being supporting batched ops on vanilla functions * some initial tests to drive TDD * gas with isolate * mint to recipient * refactor for external call and code reuse (#134) * updated interface with unlockAndExecute * update decrease (#133) * update decrease * update collect * update decrease/collect * remove delta function * update burn * fix bubbling different return types because of recursive calls * all operations only return a BalanceDelta type (#136) * temp-dev-update (#135) * checkpointing * move decrease and collect to transient storage * remove returns since they are now saved to transient storage * draft: delta closing * wip * Sra/edits (#137) * consolidate using owner, update burn * fix: accrue deltas to caller in increase * Rip Out Vanilla (#138) * rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file * fix: cleanup --------- Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Sara Reynolds <[email protected]> * using internal calls, first pass (#143) * using internal calls, first pass * fix gas tests * fix execute test * fix tests * edit mint gas test * fix mint test * fix warnings * dumb fix to test ci (#146) * dumb fix to test ci * memory limit * update gas limit to pass tests --------- Co-authored-by: gretzke <[email protected]> * some more gas snapshots (#150) * feat: posm, use salts for all positions & update permit (#148) * use position salts * use fees owed in some tests * remove claims from increase,decrease * increment token id before reading it * Revert "increment token id before reading it" This reverts commit d366d75. * owner to alice * add more mint gas tests * update comment * Owner-level ERC721 Permit (#153) * checkpointing * move decrease and collect to transient storage * remove returns since they are now saved to transient storage * draft: delta closing * wip * Sra/edits (#137) * consolidate using owner, update burn * fix: accrue deltas to caller in increase * Rip Out Vanilla (#138) * rip out vanilla and benchmark * fix gas benchmark * check posm is the locker before allowing access to external functions * restore execute tests * posm takes as 6909; remove legacy deadcode * restore tests * move helpers to the same file * move operator to NFTposm; move nonce to ERC721Permit; owner-level nonce * tests for operator/permit * snapshots * gas benchmarks for permit * test fixes * unordered nonces * fix tests / cheatcode usage * fix tests --------- Co-authored-by: Sara Reynolds <[email protected]> --------- Co-authored-by: gretzke <[email protected]> Co-authored-by: saucepoint <[email protected]> * Multicall & initialize (#154) * add multicall and an external function for initialization, with tests * test multicall contract * gas snapshot multicall * fix ci test * fix tests * forge fmt * change how msg.value is used in multicall mock --------- Co-authored-by: Sara Reynolds <[email protected]> * prep shared actions (#158) * update main (#162) * ERC721Permit cleanup (#161) * wip Solmate ERC721 * the queens dead, you may put down your arms. with this commit, i hereby lay BaseLiquidityManagement and the ideals of fee accounting finally to rest * refactor tokenId => LiquidityRange; begin separating PoolKey * move comment --------- Co-authored-by: Sara Reynolds <[email protected]> * remove old files, imports * Update src/NonfungiblePositionManager.sol Co-authored-by: saucepoint <[email protected]> * pr comments * pr comments * refactor test helpers per feedback * fix gas * remove permit * fix compiler warnings * rename to PositionManager * cache length * skip take for 0 * fix tests * Update src/interfaces/IPositionManager.sol Co-authored-by: Alice <[email protected]> * update multicall tests per feedback * remove unused imports * more unused imports * improve assertion * assert mint recipient is the payer and not the recipient * pr feedback * assert pool creation * use poolManager * remove liquidityrange imports * remove version string * pr comments, use base test setup * fuzz sqrtPrice * use fuzz, assert eq * other final rand pr comment fixes * fix off by 1 * use bound * use nextTokenId --------- Signed-off-by: saucepoint <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: saucepoint <[email protected]> Co-authored-by: 0x57 <[email protected]> Co-authored-by: 0x57 <[email protected]> Co-authored-by: gretzke <[email protected]> Co-authored-by: Alice <[email protected]>
* Decode posm params in calldata * calculate config id in assembly * use Position library * fuzz toId * fuzz calldata decoder * 2 more helper functions * decode before call * natspec in calldata decoder
* test SafeCallback * assert PM address
* slippage checks on single trades * SLippage check tests
* Optimise actions encoding * PR comment
* remove recipient from QuoteParams * run test with --isolate
* i might just be insane * additional assertion * fix pseudorandom seeding * do not bound seed * pr feedback
* poc: slippage check on increase * additional slippage tests * slippage check on mint * slippage on decrease liquidity * slippage on burn * fix tests * fix decoder; regenerate gas * forge fmt * PR feedback * rename * forge fmt
* settle with balance router * tests for commands * use currency lib and rename mock
* add CLEAR action; test gas * test clear decoding * test clear on mint, increase, and decrease * Update test/libraries/CalldataDecoder.t.sol Co-authored-by: Sara Reynolds <[email protected]> * restore old tests * forge fmt * fix tests * forge fmt * Update src/PositionManager.sol Co-authored-by: Alice <[email protected]> * natspec: decode currency and uint256 --------- Co-authored-by: Sara Reynolds <[email protected]> Co-authored-by: Alice <[email protected]>
* router decode calldata * add as functions to library * fuzz tests
Related Issue
An incorrectly written or malicious hook could revert in the remove liquidity function, bricking funds or holding user funds hostage.
Description of changes
This PR implements the MiddlewareRemoveFactory, which deploys either a new MiddlewareRemoveNoDeltas or MiddlewareRemove. This middleware protects users when removing liquidity. Removing can never revert. There is a gas limit of 5M units. If the hook returns deltas, this amount is capped to an immutable value maxFeeBips.
Rationale
I think remove protections are the best use of middlewares on-chain. While routers can protect against frontrunning during swaps and adding liquidity, they can't prevent a hook from withholding tokens. This middleware doesn't stop the hook from swapping before a user removes liquidity. While this changes the token composition withdrawn, it's a stretch to frame it as malicious towards the user. I’ve added a maxFeeBips parameter for more developer flexibility instead of banning deltas.