hardening: track and refund msg.value for position manager settle#562
hardening: track and refund msg.value for position manager settle#562mrheyday wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces transient tracking of msg.value to support partial ETH consumption during settlement actions and automatic refunds of unused ETH.
Changes:
- Add transient-storage helpers in
DeltaResolverto track and refund ETH attached to the active execution. - Update settlement flow to consume tracked ETH for native currency and reject ETH for non-native currency.
- Add
setMsgValuemodifier +_payimplementation inPositionManagerto integrate the new settlement behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/base/DeltaResolver.sol | Adds transient msg.value accounting, ETH refund helper, and enforces “no ETH with ERC20 settle” behavior. |
| src/PositionManager.sol | Wraps modifyLiquidities with transient msg.value tracking + refund, and implements _pay via Permit2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| poolManager.sync(currency); | ||
| uint256 msgValue = _getMsgValue(); | ||
| if (currency.isAddressZero()) { | ||
| poolManager.settle{value: amount}(); | ||
| if (msgValue > amount) { | ||
| _setMsgValue(msgValue - amount); | ||
| } else { | ||
| _setMsgValue(0); | ||
| } | ||
| } else { | ||
| if (msgValue != 0) { | ||
| revert UnexpectedValue(); | ||
| } |
| function _pay(Currency token, address payer, uint256 amount) internal override { | ||
| // TODO: Should we also support direct transfer? | ||
| permit2.transferFrom(payer, address(poolManager), uint160(amount), Currency.unwrap(token)); | ||
| } |
| /// @inheritdoc IPositionManager | ||
| modifier setMsgValue() { | ||
| _setMsgValue(msg.value); | ||
| _; | ||
| _refundETH(msg.sender, _getMsgValue()); | ||
| _setMsgValue(0); | ||
| } |
| /// @inheritdoc IPositionManager | ||
| modifier setMsgValue() { |
| /// @param unlockData is an encoding of actions, params, and currencies | ||
| /// @return returnData is the endocing of each actions return information |
|
|
||
| /// @inheritdoc IPositionManager | ||
| /// @param unlockData is an encoding of actions, params, and currencies | ||
| /// @return returnData is the endocing of each actions return information |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6d9987f3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // implementation of abstract function DeltaResolver._pay | ||
| function _pay(Currency token, address payer, uint256 amount) internal override { |
There was a problem hiding this comment.
_pay override
PositionManager already implements _pay(Currency,address,uint256) earlier in the contract, so adding this second function with the same signature makes the contract fail to compile due to a duplicate definition. Remove this added override or merge any intended behavior into the existing implementation.
Useful? React with 👍 / 👎.
| using PoolIdLibrary for PoolKey; | ||
| using PositionConfigLibrary for PositionConfig; |
There was a problem hiding this comment.
These newly added using directives reference PoolIdLibrary, PositionConfigLibrary, and PositionConfig, but those symbols are not imported into this file. As written, solc will reject PositionManager before any tests can run; add the missing imports or remove the unused directives.
Useful? React with 👍 / 👎.
| if (msgValue != 0) { | ||
| revert UnexpectedValue(); | ||
| } |
There was a problem hiding this comment.
When a mixed native/ERC20 batch overpays ETH, the native _settle leaves the excess in transient storage for the end-of-call refund, but any later ERC20 settle in the same batch immediately hits this UnexpectedValue branch and reverts before the refund can happen. This breaks existing native liquidity flows where msg.value is an upper bound, such as SETTLE_PAIR followed by sweeping/refunding excess ETH.
Useful? React with 👍 / 👎.
Summary
Files
Note
Based on existing hardening work from support-native-routing branch; replays cleanly and remains minimal.