From fab30d8b16f936ed8d77a6275656a8c5341e76ee Mon Sep 17 00:00:00 2001 From: diana Date: Wed, 28 Aug 2024 09:26:22 -0400 Subject: [PATCH 1/4] OZ L-09 (#308) --- src/base/Notifier.sol | 15 +++++---------- src/lens/Quoter.sol | 8 ++++---- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 59c32e8a..02617452 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -48,9 +48,8 @@ abstract contract Notifier is INotifier { if (_subscriber != NO_SUBSCRIBER) revert AlreadySubscribed(address(_subscriber)); subscriber[tokenId] = ISubscriber(newSubscriber); - bool success = _call( - address(newSubscriber), abi.encodeWithSelector(ISubscriber.notifySubscribe.selector, tokenId, config, data) - ); + bool success = + _call(address(newSubscriber), abi.encodeCall(ISubscriber.notifySubscribe, (tokenId, config, data))); if (!success) { Wrap__SubsciptionReverted.selector.bubbleUpAndRevertWith(address(newSubscriber)); @@ -87,9 +86,7 @@ abstract contract Notifier is INotifier { bool success = _call( address(_subscriber), - abi.encodeWithSelector( - ISubscriber.notifyModifyLiquidity.selector, tokenId, config, liquidityChange, feesAccrued - ) + abi.encodeCall(ISubscriber.notifyModifyLiquidity, (tokenId, config, liquidityChange, feesAccrued)) ); if (!success) { @@ -100,10 +97,8 @@ abstract contract Notifier is INotifier { function _notifyTransfer(uint256 tokenId, address previousOwner, address newOwner) internal { ISubscriber _subscriber = subscriber[tokenId]; - bool success = _call( - address(_subscriber), - abi.encodeWithSelector(ISubscriber.notifyTransfer.selector, tokenId, previousOwner, newOwner) - ); + bool success = + _call(address(_subscriber), abi.encodeCall(ISubscriber.notifyTransfer, (tokenId, previousOwner, newOwner))); if (!success) { Wrap__TransferNotificationReverted.selector.bubbleUpAndRevertWith(address(_subscriber)); diff --git a/src/lens/Quoter.sol b/src/lens/Quoter.sol index fcf63d0c..1ba06122 100644 --- a/src/lens/Quoter.sol +++ b/src/lens/Quoter.sol @@ -60,7 +60,7 @@ contract Quoter is IQuoter, SafeCallback { override returns (int128[] memory deltaAmounts, uint160 sqrtPriceX96After, uint32 initializedTicksLoaded) { - try poolManager.unlock(abi.encodeWithSelector(this._quoteExactInputSingle.selector, params)) {} + try poolManager.unlock(abi.encodeCall(this._quoteExactInputSingle, (params))) {} catch (bytes memory reason) { return _handleRevertSingle(reason); } @@ -75,7 +75,7 @@ contract Quoter is IQuoter, SafeCallback { uint32[] memory initializedTicksLoadedList ) { - try poolManager.unlock(abi.encodeWithSelector(this._quoteExactInput.selector, params)) {} + try poolManager.unlock(abi.encodeCall(this._quoteExactInput, (params))) {} catch (bytes memory reason) { return _handleRevert(reason); } @@ -87,7 +87,7 @@ contract Quoter is IQuoter, SafeCallback { override returns (int128[] memory deltaAmounts, uint160 sqrtPriceX96After, uint32 initializedTicksLoaded) { - try poolManager.unlock(abi.encodeWithSelector(this._quoteExactOutputSingle.selector, params)) {} + try poolManager.unlock(abi.encodeCall(this._quoteExactOutputSingle, (params))) {} catch (bytes memory reason) { if (params.sqrtPriceLimitX96 == 0) delete amountOutCached; return _handleRevertSingle(reason); @@ -104,7 +104,7 @@ contract Quoter is IQuoter, SafeCallback { uint32[] memory initializedTicksLoadedList ) { - try poolManager.unlock(abi.encodeWithSelector(this._quoteExactOutput.selector, params)) {} + try poolManager.unlock(abi.encodeCall(this._quoteExactOutput, (params))) {} catch (bytes memory reason) { return _handleRevert(reason); } From b1ef1c668aaf468765620805545be44c98feebfc Mon Sep 17 00:00:00 2001 From: saucepoint <98790946+saucepoint@users.noreply.github.com> Date: Wed, 28 Aug 2024 11:53:39 -0400 Subject: [PATCH 2/4] OZ-N08: unused imports (#306) * unused imports * other imports * more unused import --- src/PositionManager.sol | 7 +------ src/V4Router.sol | 2 +- src/base/BaseActionsRouter.sol | 1 - src/base/DeltaResolver.sol | 2 -- src/interfaces/IPositionManager.sol | 2 -- src/interfaces/IV4Router.sol | 2 +- src/lens/Quoter.sol | 4 ---- src/libraries/PoolTicksCounter.sol | 2 +- 8 files changed, 4 insertions(+), 18 deletions(-) diff --git a/src/PositionManager.sol b/src/PositionManager.sol index dfedc38c..6149fde3 100644 --- a/src/PositionManager.sol +++ b/src/PositionManager.sol @@ -4,14 +4,12 @@ pragma solidity ^0.8.24; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; import {PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; -import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; +import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; import {Position} from "@uniswap/v4-core/src/libraries/Position.sol"; import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientStateLibrary.sol"; -import {SafeTransferLib} from "solmate/src/utils/SafeTransferLib.sol"; -import {ERC20} from "solmate/src/tokens/ERC20.sol"; import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol"; import {ERC721Permit_v4} from "./base/ERC721Permit_v4.sol"; @@ -25,7 +23,6 @@ import {BaseActionsRouter} from "./base/BaseActionsRouter.sol"; import {Actions} from "./libraries/Actions.sol"; import {Notifier} from "./base/Notifier.sol"; import {CalldataDecoder} from "./libraries/CalldataDecoder.sol"; -import {INotifier} from "./interfaces/INotifier.sol"; import {Permit2Forwarder} from "./base/Permit2Forwarder.sol"; import {SlippageCheckLibrary} from "./libraries/SlippageCheck.sol"; import {PositionConfigId, PositionConfigIdLibrary} from "./libraries/PositionConfigId.sol"; @@ -107,8 +104,6 @@ contract PositionManager is Notifier, Permit2Forwarder { - using SafeTransferLib for *; - using CurrencyLibrary for Currency; using PoolIdLibrary for PoolKey; using PositionConfigLibrary for PositionConfig; using StateLibrary for IPoolManager; diff --git a/src/V4Router.sol b/src/V4Router.sol index 3733cca3..7e0554d7 100644 --- a/src/V4Router.sol +++ b/src/V4Router.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.19; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; -import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; +import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; diff --git a/src/base/BaseActionsRouter.sol b/src/base/BaseActionsRouter.sol index 85bca0e8..ddc5ae5f 100644 --- a/src/base/BaseActionsRouter.sol +++ b/src/base/BaseActionsRouter.sol @@ -4,7 +4,6 @@ pragma solidity ^0.8.24; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {SafeCallback} from "./SafeCallback.sol"; import {CalldataDecoder} from "../libraries/CalldataDecoder.sol"; -import {Actions} from "../libraries/Actions.sol"; import {ActionConstants} from "../libraries/ActionConstants.sol"; /// @notice Abstract contract for performing a combination of actions on Uniswap v4. diff --git a/src/base/DeltaResolver.sol b/src/base/DeltaResolver.sol index 8170ae94..b8432c3c 100644 --- a/src/base/DeltaResolver.sol +++ b/src/base/DeltaResolver.sol @@ -5,14 +5,12 @@ import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {TransientStateLibrary} from "@uniswap/v4-core/src/libraries/TransientStateLibrary.sol"; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {ImmutableState} from "./ImmutableState.sol"; -import {SafeCast} from "@uniswap/v4-core/src/libraries/SafeCast.sol"; import {ActionConstants} from "../libraries/ActionConstants.sol"; /// @notice Abstract contract used to sync, send, and settle funds to the pool manager /// @dev Note that sync() is called before any erc-20 transfer in `settle`. abstract contract DeltaResolver is ImmutableState { using TransientStateLibrary for IPoolManager; - using SafeCast for *; /// @notice Emitted trying to settle a positive delta. error DeltaNotPositive(Currency currency); diff --git a/src/interfaces/IPositionManager.sol b/src/interfaces/IPositionManager.sol index 03213c74..ceeae39d 100644 --- a/src/interfaces/IPositionManager.sol +++ b/src/interfaces/IPositionManager.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.24; -import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; -import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {PositionConfig} from "../libraries/PositionConfig.sol"; import {INotifier} from "./INotifier.sol"; diff --git a/src/interfaces/IV4Router.sol b/src/interfaces/IV4Router.sol index 6991d72d..8bde55bf 100644 --- a/src/interfaces/IV4Router.sol +++ b/src/interfaces/IV4Router.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.19; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; -import {Currency, CurrencyLibrary} from "@uniswap/v4-core/src/types/Currency.sol"; +import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; import {PathKey} from "../libraries/PathKey.sol"; /// @title IV4Router diff --git a/src/lens/Quoter.sol b/src/lens/Quoter.sol index 1ba06122..e48a4850 100644 --- a/src/lens/Quoter.sol +++ b/src/lens/Quoter.sol @@ -1,10 +1,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later pragma solidity ^0.8.20; -import {Hooks} from "@uniswap/v4-core/src/libraries/Hooks.sol"; import {TickMath} from "@uniswap/v4-core/src/libraries/TickMath.sol"; -import {IHooks} from "@uniswap/v4-core/src/interfaces/IHooks.sol"; -import {IUnlockCallback} from "@uniswap/v4-core/src/interfaces/callback/IUnlockCallback.sol"; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; import {Currency} from "@uniswap/v4-core/src/types/Currency.sol"; @@ -17,7 +14,6 @@ import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; import {SafeCallback} from "../base/SafeCallback.sol"; contract Quoter is IQuoter, SafeCallback { - using Hooks for IHooks; using PoolIdLibrary for PoolKey; using PathKeyLib for PathKey; using StateLibrary for IPoolManager; diff --git a/src/libraries/PoolTicksCounter.sol b/src/libraries/PoolTicksCounter.sol index 7420ffd5..49b3ff36 100644 --- a/src/libraries/PoolTicksCounter.sol +++ b/src/libraries/PoolTicksCounter.sol @@ -3,7 +3,7 @@ pragma solidity >=0.8.20; import {IPoolManager} from "@uniswap/v4-core/src/interfaces/IPoolManager.sol"; import {PoolKey} from "@uniswap/v4-core/src/types/PoolKey.sol"; -import {PoolId, PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; +import {PoolIdLibrary} from "@uniswap/v4-core/src/types/PoolId.sol"; import {StateLibrary} from "@uniswap/v4-core/src/libraries/StateLibrary.sol"; library PoolTicksCounter { From 7e773b3bbf89d081629aed81ccb5e277cb1f6118 Mon Sep 17 00:00:00 2001 From: diana Date: Wed, 28 Aug 2024 14:55:33 -0400 Subject: [PATCH 3/4] OZ L-08 (#313) --- src/lens/Quoter.sol | 11 ++++++----- src/libraries/CalldataDecoder.sol | 4 ++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/lens/Quoter.sol b/src/lens/Quoter.sol index e48a4850..17443b6b 100644 --- a/src/lens/Quoter.sol +++ b/src/lens/Quoter.sol @@ -21,9 +21,10 @@ contract Quoter is IQuoter, SafeCallback { /// @dev cache used to check a safety condition in exact output swaps. uint128 private amountOutCached; - /// @dev min valid reason is 3-words long - /// @dev int128[2] + sqrtPriceX96After padded to 32bytes + intializeTicksLoaded padded to 32bytes - uint256 internal constant MINIMUM_VALID_RESPONSE_LENGTH = 96; + /// @dev min valid reason is 6-words long (192 bytes) + /// @dev int128[2] includes 32 bytes for offset, 32 bytes for length, and 32 bytes for each element + /// @dev Plus sqrtPriceX96After padded to 32 bytes and initializedTicksLoaded padded to 32 bytes + uint256 internal constant MINIMUM_VALID_RESPONSE_LENGTH = 192; struct QuoteResult { int128[] deltaAmounts; @@ -266,7 +267,7 @@ contract Quoter is IQuoter, SafeCallback { /// @dev quote an ExactOutput swap on a pool, then revert with the result function _quoteExactOutputSingle(QuoteExactSingleParams calldata params) public selfOnly returns (bytes memory) { - // if no price limit has been specified, cache the output amount for comparison in the swap callback + // if no price limit has been specified, cache the output amount for comparison inside the _swap function if (params.sqrtPriceLimitX96 == 0) amountOutCached = params.exactAmount; (, int24 tickBefore,,) = poolManager.getSlot0(params.poolKey.toId()); @@ -293,7 +294,7 @@ contract Quoter is IQuoter, SafeCallback { } /// @dev Execute a swap and return the amounts delta, as well as relevant pool state - /// @notice if amountSpecified > 0, the swap is exactInput, otherwise exactOutput + /// @notice if amountSpecified < 0, the swap is exactInput, otherwise exactOutput function _swap( PoolKey memory poolKey, bool zeroForOne, diff --git a/src/libraries/CalldataDecoder.sol b/src/libraries/CalldataDecoder.sol index 197987d3..8041eb64 100644 --- a/src/libraries/CalldataDecoder.sol +++ b/src/libraries/CalldataDecoder.sol @@ -14,7 +14,7 @@ library CalldataDecoder { /// @notice equivalent to SliceOutOfBounds.selector bytes4 constant SLICE_ERROR_SELECTOR = 0x3b99b53d; - /// @dev equivalent to: abi.decode(params, (uint256[], bytes[])) in calldata + /// @dev equivalent to: abi.decode(params, (bytes, bytes[])) in calldata function decodeActionsRouterParams(bytes calldata _bytes) internal pure @@ -147,7 +147,7 @@ library CalldataDecoder { } } - /// @dev equivalent to: abi.decode(params, (IV4Router.ExactInputSingleParams)) + /// @dev equivalent to: abi.decode(params, (IV4Router.ExactOutputSingleParams)) function decodeSwapExactOutSingleParams(bytes calldata params) internal pure From e2c3d0b51f7ffacdbad44197f7ab24ea0b50df17 Mon Sep 17 00:00:00 2001 From: saucepoint <98790946+saucepoint@users.noreply.github.com> Date: Wed, 28 Aug 2024 17:27:17 -0400 Subject: [PATCH 4/4] OZ-L01: Revokable Unordered Nonces (#304) * revoke nonce at the abstract contract * test revokeNonce in ERC721Permit * typo --- ...tionManager_multicall_initialize_mint.snap | 2 +- .forge-snapshots/PositionManager_permit.snap | 2 +- ...PositionManager_permit_secondPosition.snap | 2 +- .../PositionManager_permit_twice.snap | 2 +- src/base/UnorderedNonce.sol | 7 +++++++ test/UnorderedNonce.t.sol | 12 +++++++++++ .../ERC721Permit.permitForAll.t.sol | 19 +++++++++++++++++ test/position-managers/Permit.t.sol | 21 +++++++++++++++++++ 8 files changed, 63 insertions(+), 4 deletions(-) diff --git a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap index e856a5c2..8a24ee62 100644 --- a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap +++ b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap @@ -1 +1 @@ -421127 \ No newline at end of file +421104 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit.snap b/.forge-snapshots/PositionManager_permit.snap index 81715a67..e62c334b 100644 --- a/.forge-snapshots/PositionManager_permit.snap +++ b/.forge-snapshots/PositionManager_permit.snap @@ -1 +1 @@ -79484 \ No newline at end of file +79506 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_secondPosition.snap b/.forge-snapshots/PositionManager_permit_secondPosition.snap index bd214aa7..a0c684a2 100644 --- a/.forge-snapshots/PositionManager_permit_secondPosition.snap +++ b/.forge-snapshots/PositionManager_permit_secondPosition.snap @@ -1 +1 @@ -62372 \ No newline at end of file +62394 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_twice.snap b/.forge-snapshots/PositionManager_permit_twice.snap index c0e053f6..2439b179 100644 --- a/.forge-snapshots/PositionManager_permit_twice.snap +++ b/.forge-snapshots/PositionManager_permit_twice.snap @@ -1 +1 @@ -45260 \ No newline at end of file +45282 \ No newline at end of file diff --git a/src/base/UnorderedNonce.sol b/src/base/UnorderedNonce.sol index a1efe769..75a34fe4 100644 --- a/src/base/UnorderedNonce.sol +++ b/src/base/UnorderedNonce.sol @@ -21,4 +21,11 @@ contract UnorderedNonce { uint256 flipped = nonces[owner][wordPos] ^= bit; if (flipped & bit == 0) revert NonceAlreadyUsed(); } + + /// @notice Revoke a nonce by spending it, preventing it from being used again + /// @dev Used in cases where a valid nonce has not been broadcasted onchain, and the owner wants to revoke the validity of the nonce + /// @dev payable so it can be multicalled with native-token related actions + function revokeNonce(uint256 nonce) external payable { + _useUnorderedNonce(msg.sender, nonce); + } } diff --git a/test/UnorderedNonce.t.sol b/test/UnorderedNonce.t.sol index 9683c763..d3939fd2 100644 --- a/test/UnorderedNonce.t.sol +++ b/test/UnorderedNonce.t.sol @@ -107,4 +107,16 @@ contract UnorderedNonceTest is Test { unorderedNonce.spendNonce(address(this), second); } } + + function test_fuzz_revokeNonce(uint256 nonce) public { + unorderedNonce.revokeNonce(nonce); + vm.expectRevert(UnorderedNonce.NonceAlreadyUsed.selector); + unorderedNonce.revokeNonce(nonce); + } + + function test_fuzz_revokeNonce_twoNonces(uint256 first, uint256 second) public { + unorderedNonce.revokeNonce(first); + if (first == second) vm.expectRevert(UnorderedNonce.NonceAlreadyUsed.selector); + unorderedNonce.revokeNonce(second); + } } diff --git a/test/erc721Permit/ERC721Permit.permitForAll.t.sol b/test/erc721Permit/ERC721Permit.permitForAll.t.sol index 6d5f4be3..26c97a46 100644 --- a/test/erc721Permit/ERC721Permit.permitForAll.t.sol +++ b/test/erc721Permit/ERC721Permit.permitForAll.t.sol @@ -297,6 +297,25 @@ contract ERC721PermitForAllTest is Test { vm.stopPrank(); } + /// @notice revoking a nonce prevents it from being used in permitForAll() + function test_fuzz_erc721PermitForAll_revokedNonceUsed(uint256 nonce) public { + // alice revokes the nonce + vm.prank(alice); + erc721Permit.revokeNonce(nonce); + + uint256 deadline = block.timestamp; + bytes32 digest = _getPermitForAllDigest(bob, true, nonce, deadline); + // alice signs a permit for bob + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + // Nonce does not work with permitForAll + vm.startPrank(bob); + vm.expectRevert(UnorderedNonce.NonceAlreadyUsed.selector); + erc721Permit.permitForAll(alice, bob, true, deadline, nonce, signature); + vm.stopPrank(); + } + // Helpers related to permitForAll function _permitForAll(uint256 privateKey, address owner, address operator, bool approved, uint256 nonce) internal diff --git a/test/position-managers/Permit.t.sol b/test/position-managers/Permit.t.sol index 6cf03c3f..2219e3f0 100644 --- a/test/position-managers/Permit.t.sol +++ b/test/position-managers/Permit.t.sol @@ -217,6 +217,27 @@ contract PermitTest is Test, PosmTestSetup { vm.stopPrank(); } + /// @notice revoking a nonce prevents it from being used in permit() + function test_fuzz_noPermit_revokeRevert(uint256 nonce) public { + uint256 liquidityAlice = 1e18; + vm.prank(alice); + mint(config, liquidityAlice, alice, ZERO_BYTES); + uint256 tokenIdAlice = lpm.nextTokenId() - 1; + + // alice revokes the nonce + vm.prank(alice); + lpm.revokeNonce(nonce); + + // alice gives bob spender permissions + bytes32 digest = getDigest(bob, tokenIdAlice, nonce, block.timestamp + 1); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePK, digest); + bytes memory signature = abi.encodePacked(r, s, v); + + vm.expectRevert(UnorderedNonce.NonceAlreadyUsed.selector); + lpm.permit(bob, tokenIdAlice, block.timestamp + 1, nonce, signature); + } + // Bob can use alice's signature to permit & decrease liquidity function test_permit_operatorSelfPermit() public { uint256 liquidityAlice = 1e18;