From 4224c5b7c97723c7b5a8bf7e6ab36750ddddbede Mon Sep 17 00:00:00 2001 From: saucepoint Date: Sun, 30 Jun 2024 22:19:58 -0400 Subject: [PATCH] check posm is the locker before allowing access to external functions --- .../autocompound_exactUnclaimedFees.snap | 2 +- ...exactUnclaimedFees_exactCustodiedFees.snap | 2 +- .../autocompound_excessFeesCredit.snap | 2 +- .forge-snapshots/decreaseLiquidity_erc20.snap | 2 +- .../decreaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/increaseLiquidity_erc20.snap | 2 +- .../increaseLiquidity_erc6909.snap | 2 +- .forge-snapshots/mintWithLiquidity.snap | 2 +- contracts/NonfungiblePositionManager.sol | 28 +++++++++++-------- contracts/base/BaseLiquidityManagement.sol | 8 ++++-- .../INonfungiblePositionManager.sol | 2 ++ .../libraries/TransientLiquidityDelta.sol | 11 ++++++++ 12 files changed, 43 insertions(+), 22 deletions(-) diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees.snap index 6632f988..8cace02e 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees.snap @@ -1 +1 @@ -299380 \ No newline at end of file +299532 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap index 6e4bf338..68ec6575 100644 --- a/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap +++ b/.forge-snapshots/autocompound_exactUnclaimedFees_exactCustodiedFees.snap @@ -1 +1 @@ -238844 \ No newline at end of file +238996 \ No newline at end of file diff --git a/.forge-snapshots/autocompound_excessFeesCredit.snap b/.forge-snapshots/autocompound_excessFeesCredit.snap index ef8374e7..c2107d3b 100644 --- a/.forge-snapshots/autocompound_excessFeesCredit.snap +++ b/.forge-snapshots/autocompound_excessFeesCredit.snap @@ -1 +1 @@ -319919 \ No newline at end of file +320071 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc20.snap b/.forge-snapshots/decreaseLiquidity_erc20.snap index 4850a5b4..ca417da9 100644 --- a/.forge-snapshots/decreaseLiquidity_erc20.snap +++ b/.forge-snapshots/decreaseLiquidity_erc20.snap @@ -1 +1 @@ -214439 \ No newline at end of file +214591 \ No newline at end of file diff --git a/.forge-snapshots/decreaseLiquidity_erc6909.snap b/.forge-snapshots/decreaseLiquidity_erc6909.snap index 24d8e85d..597e3839 100644 --- a/.forge-snapshots/decreaseLiquidity_erc6909.snap +++ b/.forge-snapshots/decreaseLiquidity_erc6909.snap @@ -1 +1 @@ -214451 \ No newline at end of file +214603 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc20.snap b/.forge-snapshots/increaseLiquidity_erc20.snap index 18129560..3904f832 100644 --- a/.forge-snapshots/increaseLiquidity_erc20.snap +++ b/.forge-snapshots/increaseLiquidity_erc20.snap @@ -1 +1 @@ -194596 \ No newline at end of file +194748 \ No newline at end of file diff --git a/.forge-snapshots/increaseLiquidity_erc6909.snap b/.forge-snapshots/increaseLiquidity_erc6909.snap index 3d4e376c..a9e8eac5 100644 --- a/.forge-snapshots/increaseLiquidity_erc6909.snap +++ b/.forge-snapshots/increaseLiquidity_erc6909.snap @@ -1 +1 @@ -194608 \ No newline at end of file +194760 \ No newline at end of file diff --git a/.forge-snapshots/mintWithLiquidity.snap b/.forge-snapshots/mintWithLiquidity.snap index cfba5ed8..c4d1bd5c 100644 --- a/.forge-snapshots/mintWithLiquidity.snap +++ b/.forge-snapshots/mintWithLiquidity.snap @@ -1 +1 @@ -513548 \ No newline at end of file +513700 \ No newline at end of file diff --git a/contracts/NonfungiblePositionManager.sol b/contracts/NonfungiblePositionManager.sol index bc385874..b6318f31 100644 --- a/contracts/NonfungiblePositionManager.sol +++ b/contracts/NonfungiblePositionManager.sol @@ -38,8 +38,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit // maps the ERC721 tokenId to the keys that uniquely identify a liquidity position (owner, range) mapping(uint256 tokenId => TokenPosition position) public tokenPositions; - // TODO: TSTORE this jawn + // TODO: TSTORE these jawns address internal msgSender; + bool internal unlockedByThis; // TODO: Context is inherited through ERC721 and will be not useful to use _msgSender() which will be address(this) with our current mutlicall. function _msgSenderInternal() internal override returns (address) { @@ -53,6 +54,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit function unlockAndExecute(bytes[] memory data, Currency[] memory currencies) public returns (int128[] memory) { msgSender = msg.sender; + unlockedByThis = true; return abi.decode(manager.unlock(abi.encode(data, currencies)), (int128[])); } @@ -88,8 +90,7 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit uint256 deadline, address owner, bytes calldata hookData - ) external payable { - // TODO: security -- what happens if PoolManager is already unlocked by UR??? + ) external payable onlyIfUnlocked { _increaseLiquidity(owner, range, liquidity, hookData); // mint receipt token @@ -121,20 +122,20 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit function increaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) external isAuthorizedForToken(tokenId) + onlyIfUnlocked { TokenPosition memory tokenPos = tokenPositions[tokenId]; - // TODO: security -- what happens if PoolManager is already unlocked by UR??? _increaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); } function decreaseLiquidity(uint256 tokenId, uint256 liquidity, bytes calldata hookData, bool claims) external isAuthorizedForToken(tokenId) + onlyIfUnlocked { TokenPosition memory tokenPos = tokenPositions[tokenId]; - // TODO: security -- what happens if PoolManager is already unlocked by UR??? _decreaseLiquidity(tokenPos.owner, tokenPos.range, liquidity, hookData); } @@ -151,15 +152,13 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit } // TODO: in v3, we can partially collect fees, but what was the usecase here? - function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) external { + function collect(uint256 tokenId, address recipient, bytes calldata hookData, bool claims) + external + onlyIfUnlocked + { TokenPosition memory tokenPos = tokenPositions[tokenId]; - // TODO: security -- what happens if PoolManager is already unlocked by UR??? - _collect(recipient, tokenPos.range, hookData); - if (recipient != _msgSenderInternal()) { - tokenPos.range.poolKey.currency0.close(manager, recipient); - tokenPos.range.poolKey.currency1.close(manager, recipient); - } + _collect(recipient, tokenPos.owner, tokenPos.range, hookData); } function feesOwed(uint256 tokenId) external view returns (uint256 token0Owed, uint256 token1Owed) { @@ -190,4 +189,9 @@ contract NonfungiblePositionManager is INonfungiblePositionManager, BaseLiquidit require(msg.sender == address(this) || _isApprovedOrOwner(msg.sender, tokenId), "Not approved"); _; } + + modifier onlyIfUnlocked() { + if (!unlockedByThis) revert MustBeUnlockedByThisContract(); + _; + } } diff --git a/contracts/base/BaseLiquidityManagement.sol b/contracts/base/BaseLiquidityManagement.sol index c1c1308b..b6f5d0e2 100644 --- a/contracts/base/BaseLiquidityManagement.sol +++ b/contracts/base/BaseLiquidityManagement.sol @@ -206,7 +206,7 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb } // The recipient may not be the original owner. - function _collect(address owner, LiquidityRange memory range, bytes memory hookData) internal { + function _collect(address recipient, address owner, LiquidityRange memory range, bytes memory hookData) internal { BalanceDelta callerDelta; BalanceDelta thisDelta; Position storage position = positions[owner][range.toId()]; @@ -233,7 +233,11 @@ abstract contract BaseLiquidityManagement is IBaseLiquidityManagement, SafeCallb callerDelta = callerDelta + tokensOwed; thisDelta = thisDelta - tokensOwed; - callerDelta.flush(owner, range.poolKey.currency0, range.poolKey.currency1); + if (recipient == _msgSenderInternal()) { + callerDelta.flush(recipient, range.poolKey.currency0, range.poolKey.currency1); + } else { + callerDelta.closeDelta(manager, recipient, range.poolKey.currency0, range.poolKey.currency1); + } thisDelta.flush(address(this), range.poolKey.currency0, range.poolKey.currency1); position.clearTokensOwed(); diff --git a/contracts/interfaces/INonfungiblePositionManager.sol b/contracts/interfaces/INonfungiblePositionManager.sol index 03c83334..94cf9206 100644 --- a/contracts/interfaces/INonfungiblePositionManager.sol +++ b/contracts/interfaces/INonfungiblePositionManager.sol @@ -11,6 +11,8 @@ interface INonfungiblePositionManager { LiquidityRange range; } + error MustBeUnlockedByThisContract(); + // NOTE: more gas efficient as LiquidityAmounts is used offchain function mint( LiquidityRange calldata position, diff --git a/contracts/libraries/TransientLiquidityDelta.sol b/contracts/libraries/TransientLiquidityDelta.sol index 83ba3139..ccf520d4 100644 --- a/contracts/libraries/TransientLiquidityDelta.sol +++ b/contracts/libraries/TransientLiquidityDelta.sol @@ -74,6 +74,17 @@ library TransientLiquidityDelta { } } + function closeDelta( + BalanceDelta delta, + IPoolManager manager, + address holder, + Currency currency0, + Currency currency1 + ) internal { + close(currency0, manager, holder); + close(currency1, manager, holder); + } + function getBalanceDelta(address holder, Currency currency0, Currency currency1) internal view