From 255b20ebc574a9a50bfe44b2e175ff01315721e1 Mon Sep 17 00:00:00 2001 From: saucepoint <98790946+saucepoint@users.noreply.github.com> Date: Wed, 4 Sep 2024 16:14:12 -0400 Subject: [PATCH] OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, Unsubscribe Minimum Gas Limit (#318) * check subscriber code length * revert unsubscribe if not subscribed * check if subscriber has code before notifying * custom reverts * SB-M78: minimum gas limit on unsubscribe * natspec on unsubscribe for SB M78 * Update src/interfaces/INotifier.sol Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com> * use constructor for subscriber gas limit * natspec for unsubscribeGasLimit * document EIP-150 instead * Update src/base/Notifier.sol Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com> * forge fmt lol * gas benchmarks for subscribe/unsubscribe * gas optimize unsubscribe check --------- Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com> --- ...tionManager_multicall_initialize_mint.snap | 2 +- .forge-snapshots/PositionManager_permit.snap | 2 +- ...PositionManager_permit_secondPosition.snap | 2 +- .../PositionManager_permit_twice.snap | 2 +- .../PositionManager_subscribe.snap | 1 + .../PositionManager_unsubscribe.snap | 1 + script/DeployPosm.s.sol | 9 +- src/PositionManager.sol | 3 +- src/base/Notifier.sol | 24 +-- src/interfaces/INotifier.sol | 11 ++ src/interfaces/ISubscriber.sol | 3 + .../PositionManager.gas.t.sol | 16 ++ .../PositionManager.notifier.t.sol | 144 +++++++++++++++++- test/shared/PosmTestSetup.sol | 2 +- 14 files changed, 204 insertions(+), 18 deletions(-) create mode 100644 .forge-snapshots/PositionManager_subscribe.snap create mode 100644 .forge-snapshots/PositionManager_unsubscribe.snap diff --git a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap index f7ee51aa0..f073e2670 100644 --- a/.forge-snapshots/PositionManager_multicall_initialize_mint.snap +++ b/.forge-snapshots/PositionManager_multicall_initialize_mint.snap @@ -1 +1 @@ -420775 \ No newline at end of file +420753 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit.snap b/.forge-snapshots/PositionManager_permit.snap index b852adad4..d71fc9167 100644 --- a/.forge-snapshots/PositionManager_permit.snap +++ b/.forge-snapshots/PositionManager_permit.snap @@ -1 +1 @@ -79492 \ No newline at end of file +79470 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_secondPosition.snap b/.forge-snapshots/PositionManager_permit_secondPosition.snap index dbca4d250..bf069bcae 100644 --- a/.forge-snapshots/PositionManager_permit_secondPosition.snap +++ b/.forge-snapshots/PositionManager_permit_secondPosition.snap @@ -1 +1 @@ -62380 \ No newline at end of file +62358 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_permit_twice.snap b/.forge-snapshots/PositionManager_permit_twice.snap index 8bef075e2..912820cbe 100644 --- a/.forge-snapshots/PositionManager_permit_twice.snap +++ b/.forge-snapshots/PositionManager_permit_twice.snap @@ -1 +1 @@ -45268 \ No newline at end of file +45246 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_subscribe.snap b/.forge-snapshots/PositionManager_subscribe.snap new file mode 100644 index 000000000..dec6506f4 --- /dev/null +++ b/.forge-snapshots/PositionManager_subscribe.snap @@ -0,0 +1 @@ +88474 \ No newline at end of file diff --git a/.forge-snapshots/PositionManager_unsubscribe.snap b/.forge-snapshots/PositionManager_unsubscribe.snap new file mode 100644 index 000000000..4efc0fb76 --- /dev/null +++ b/.forge-snapshots/PositionManager_unsubscribe.snap @@ -0,0 +1 @@ +62639 \ No newline at end of file diff --git a/script/DeployPosm.s.sol b/script/DeployPosm.s.sol index 92084bdca..3ce242504 100644 --- a/script/DeployPosm.s.sol +++ b/script/DeployPosm.s.sol @@ -12,10 +12,15 @@ import {IAllowanceTransfer} from "permit2/src/interfaces/IAllowanceTransfer.sol" contract DeployPosmTest is Script { function setUp() public {} - function run(address poolManager, address permit2) public returns (PositionManager posm) { + function run(address poolManager, address permit2, uint256 unsubscribeGasLimit) + public + returns (PositionManager posm) + { vm.startBroadcast(); - posm = new PositionManager{salt: hex"03"}(IPoolManager(poolManager), IAllowanceTransfer(permit2)); + posm = new PositionManager{salt: hex"03"}( + IPoolManager(poolManager), IAllowanceTransfer(permit2), unsubscribeGasLimit + ); console2.log("PositionManager", address(posm)); vm.stopBroadcast(); diff --git a/src/PositionManager.sol b/src/PositionManager.sol index 9c1533479..5bbeccd22 100644 --- a/src/PositionManager.sol +++ b/src/PositionManager.sol @@ -125,10 +125,11 @@ contract PositionManager is return positionConfigs[tokenId]; } - constructor(IPoolManager _poolManager, IAllowanceTransfer _permit2) + constructor(IPoolManager _poolManager, IAllowanceTransfer _permit2, uint256 _unsubscribeGasLimit) BaseActionsRouter(_poolManager) Permit2Forwarder(_permit2) ERC721Permit_v4("Uniswap V4 Positions NFT", "UNI-V4-POSM") + Notifier(_unsubscribeGasLimit) {} /// @notice Reverts if the deadline has passed diff --git a/src/base/Notifier.sol b/src/base/Notifier.sol index 0b7d1d716..bb94f73a7 100644 --- a/src/base/Notifier.sol +++ b/src/base/Notifier.sol @@ -4,26 +4,27 @@ pragma solidity ^0.8.0; import {ISubscriber} from "../interfaces/ISubscriber.sol"; import {PositionConfig} from "../libraries/PositionConfig.sol"; import {PositionConfigId, PositionConfigIdLibrary} from "../libraries/PositionConfigId.sol"; -import {BipsLibrary} from "../libraries/BipsLibrary.sol"; import {INotifier} from "../interfaces/INotifier.sol"; import {CustomRevert} from "@uniswap/v4-core/src/libraries/CustomRevert.sol"; import {BalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol"; /// @notice Notifier is used to opt in to sending updates to external contracts about position modifications or transfers abstract contract Notifier is INotifier { - using BipsLibrary for uint256; using CustomRevert for bytes4; using PositionConfigIdLibrary for PositionConfigId; ISubscriber private constant NO_SUBSCRIBER = ISubscriber(address(0)); - // a percentage of the block.gaslimit denoted in BPS, used as the gas limit for subscriber calls - // 100 bps is 1%, at 30M gas, the limit is 300K - uint256 private constant BLOCK_LIMIT_BPS = 100; + /// @inheritdoc INotifier + uint256 public immutable unsubscribeGasLimit; /// @inheritdoc INotifier mapping(uint256 tokenId => ISubscriber subscriber) public subscriber; + constructor(uint256 _unsubscribeGasLimit) { + unsubscribeGasLimit = _unsubscribeGasLimit; + } + /// @notice Only allow callers that are approved as spenders or operators of the tokenId /// @dev to be implemented by the parent contract (PositionManager) /// @param caller the address of the caller @@ -68,6 +69,7 @@ abstract contract Notifier is INotifier { onlyIfApproved(msg.sender, tokenId) onlyValidConfig(tokenId, config) { + if (!_positionConfigs(tokenId).hasSubscriber()) NotSubscribed.selector.revertWith(); _unsubscribe(tokenId, config); } @@ -77,10 +79,13 @@ abstract contract Notifier is INotifier { delete subscriber[tokenId]; - // A gas limit and a try-catch block are used to protect users from a malicious subscriber. - // Users should always be able to unsubscribe, not matter how the subscriber behaves. - uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS); - try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config) {} catch {} + if (address(_subscriber).code.length > 0) { + // require that the remaining gas is sufficient to notify the subscriber + // otherwise, users can select a gas limit where .notifyUnsubscribe hits OutOfGas yet the + // transaction/unsubscription can still succeed + if (gasleft() < unsubscribeGasLimit) GasLimitTooLow.selector.revertWith(); + try _subscriber.notifyUnsubscribe{gas: unsubscribeGasLimit}(tokenId, config) {} catch {} + } emit Unsubscription(tokenId, address(_subscriber)); } @@ -115,6 +120,7 @@ abstract contract Notifier is INotifier { } function _call(address target, bytes memory encodedCall) internal returns (bool success) { + if (target.code.length == 0) NoCodeSubscriber.selector.revertWith(); assembly ("memory-safe") { success := call(gas(), target, 0, add(encodedCall, 0x20), mload(encodedCall), 0, 0) } diff --git a/src/interfaces/INotifier.sol b/src/interfaces/INotifier.sol index 885a93289..17bb8f961 100644 --- a/src/interfaces/INotifier.sol +++ b/src/interfaces/INotifier.sol @@ -6,6 +6,12 @@ import {ISubscriber} from "./ISubscriber.sol"; /// @notice This interface is used to opt in to sending updates to external contracts about position modifications or transfers interface INotifier { + /// @notice Thrown when unsubscribing without a subscriber + error NotSubscribed(); + /// @notice Thrown when a subscriber does not have code + error NoCodeSubscriber(); + /// @notice Thrown when a user specifies a gas limit too low to avoid valid unsubscribe notifications + error GasLimitTooLow(); /// @notice Wraps the revert message of the subscriber contract on a reverting subscription error Wrap__SubscriptionReverted(address subscriber, bytes reason); /// @notice Wraps the revert message of the subscriber contract on a reverting modify liquidity notification @@ -39,6 +45,7 @@ interface INotifier { /// @notice Removes the subscriber from receiving notifications for a respective position /// @param tokenId the ERC721 tokenId /// @param config the corresponding PositionConfig for the tokenId + /// @dev Callers must specify a high gas limit (remaining gas should be higher than subscriberGasLimit) such that the subscriber can be notified /// @dev payable so it can be multicalled with NATIVE related actions /// @dev Must always allow a user to unsubscribe. In the case of a malicious subscriber, a user can always unsubscribe safely, ensuring liquidity is always modifiable. function unsubscribe(uint256 tokenId, PositionConfig calldata config) external payable; @@ -47,4 +54,8 @@ interface INotifier { /// @param tokenId the ERC721 tokenId /// @return bool whether or not the position has a subscriber function hasSubscriber(uint256 tokenId) external view returns (bool); + + /// @notice Returns and determines the maximum allowable gas-used for notifying unsubscribe + /// @return uint256 the maximum gas limit when notifying a subscriber's `notifyUnsubscribe` function + function unsubscribeGasLimit() external view returns (uint256); } diff --git a/src/interfaces/ISubscriber.sol b/src/interfaces/ISubscriber.sol index ceab3ac81..a1393c7a9 100644 --- a/src/interfaces/ISubscriber.sol +++ b/src/interfaces/ISubscriber.sol @@ -10,6 +10,9 @@ interface ISubscriber { /// @param config details about the position /// @param data additional data passed in by the caller function notifySubscribe(uint256 tokenId, PositionConfig memory config, bytes memory data) external; + /// @notice Called when a position unsubscribes from the subscriber + /// @dev This call's gas is capped at `unsubscribeGasLimit` (set at deployment) + /// @dev Because of EIP-150, solidity may only allocate 63/64 of gasleft() /// @param tokenId the token ID of the position /// @param config details about the position function notifyUnsubscribe(uint256 tokenId, PositionConfig memory config) external; diff --git a/test/position-managers/PositionManager.gas.t.sol b/test/position-managers/PositionManager.gas.t.sol index 161321eac..2463b7102 100644 --- a/test/position-managers/PositionManager.gas.t.sol +++ b/test/position-managers/PositionManager.gas.t.sol @@ -24,6 +24,7 @@ import {IMulticall_v4} from "../../src/interfaces/IMulticall_v4.sol"; import {Planner, Plan} from "../shared/Planner.sol"; import {PosmTestSetup} from "../shared/PosmTestSetup.sol"; import {ActionConstants} from "../../src/libraries/ActionConstants.sol"; +import {MockSubscriber} from "../mocks/MockSubscriber.sol"; contract PosMGasTest is Test, PosmTestSetup, GasSnapshot { using FixedPointMathLib for uint256; @@ -43,6 +44,8 @@ contract PosMGasTest is Test, PosmTestSetup, GasSnapshot { PositionConfig config; PositionConfig configNative; + MockSubscriber sub; + function setUp() public { (alice, alicePK) = makeAddrAndKey("ALICE"); (bob, bobPK) = makeAddrAndKey("BOB"); @@ -68,6 +71,8 @@ contract PosMGasTest is Test, PosmTestSetup, GasSnapshot { // define a reusable range config = PositionConfig({poolKey: key, tickLower: -300, tickUpper: 300}); configNative = PositionConfig({poolKey: nativeKey, tickLower: -300, tickUpper: 300}); + + sub = new MockSubscriber(lpm); } function test_gas_mint_withClose() public { @@ -850,4 +855,15 @@ contract PosMGasTest is Test, PosmTestSetup, GasSnapshot { lpm.modifyLiquidities(calls, _deadline); snapLastCall("PositionManager_decrease_take_take"); } + + function test_gas_subscribe_unsubscribe() public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 1e18, ActionConstants.MSG_SENDER, ZERO_BYTES); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + snapLastCall("PositionManager_subscribe"); + + lpm.unsubscribe(tokenId, config); + snapLastCall("PositionManager_unsubscribe"); + } } diff --git a/test/position-managers/PositionManager.notifier.t.sol b/test/position-managers/PositionManager.notifier.t.sol index ea7931152..d06ed52d5 100644 --- a/test/position-managers/PositionManager.notifier.t.sol +++ b/test/position-managers/PositionManager.notifier.t.sol @@ -97,6 +97,22 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { assertEq(sub.notifySubscribeCount(), 1); } + /// @notice Revert when subscribing to an address without code + function test_subscribe_revert_empty(address _subscriber) public { + vm.assume(_subscriber.code.length == 0); + + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, alice, ZERO_BYTES); + + // approve this contract to operate on alices liq + vm.startPrank(alice); + lpm.approve(address(this), tokenId); + vm.stopPrank(); + + vm.expectRevert(INotifier.NoCodeSubscriber.selector); + lpm.subscribe(tokenId, config, _subscriber, ZERO_BYTES); + } + function test_subscribe_revertsWithAlreadySubscribed() public { uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, alice, ZERO_BYTES); @@ -145,6 +161,28 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { assertEq(sub.notifyModifyLiquidityCount(), 10); } + function test_notifyModifyLiquidity_selfDestruct_revert() public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, alice, ZERO_BYTES); + + // approve this contract to operate on alices liq + vm.startPrank(alice); + lpm.approve(address(this), tokenId); + vm.stopPrank(); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + + assertEq(lpm.hasSubscriber(tokenId), true); + assertEq(address(lpm.subscriber(tokenId)), address(sub)); + + // simulate selfdestruct by etching the bytecode to 0 + vm.etch(address(sub), ZERO_BYTES); + + uint256 liquidityToAdd = 10e18; + vm.expectRevert(INotifier.NoCodeSubscriber.selector); + increaseLiquidity(tokenId, config, liquidityToAdd, ZERO_BYTES); + } + function test_notifyModifyLiquidity_args() public { uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, alice, ZERO_BYTES); @@ -192,6 +230,26 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { assertEq(sub.notifyTransferCount(), 1); } + function test_notifyTransfer_withTransferFrom_selfDestruct_revert() public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, alice, ZERO_BYTES); + + // approve this contract to operate on alices liq + vm.startPrank(alice); + lpm.approve(address(this), tokenId); + vm.stopPrank(); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + assertEq(lpm.hasSubscriber(tokenId), true); + assertEq(address(lpm.subscriber(tokenId)), address(sub)); + + // simulate selfdestruct by etching the bytecode to 0 + vm.etch(address(sub), ZERO_BYTES); + + vm.expectRevert(INotifier.NoCodeSubscriber.selector); + lpm.transferFrom(alice, bob, tokenId); + } + function test_notifyTransfer_withSafeTransferFrom_succeeds() public { uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, alice, ZERO_BYTES); @@ -211,6 +269,26 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { assertEq(sub.notifyTransferCount(), 1); } + function test_notifyTransfer_withSafeTransferFrom_selfDestruct_revert() public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, alice, ZERO_BYTES); + + // approve this contract to operate on alices liq + vm.startPrank(alice); + lpm.approve(address(this), tokenId); + vm.stopPrank(); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + assertEq(lpm.hasSubscriber(tokenId), true); + assertEq(address(lpm.subscriber(tokenId)), address(sub)); + + // simulate selfdestruct by etching the bytecode to 0 + vm.etch(address(sub), ZERO_BYTES); + + vm.expectRevert(INotifier.NoCodeSubscriber.selector); + lpm.safeTransferFrom(alice, bob, tokenId); + } + function test_notifyTransfer_withSafeTransferFromData_succeeds() public { uint256 tokenId = lpm.nextTokenId(); mint(config, 100e18, alice, ZERO_BYTES); @@ -268,6 +346,26 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { assertEq(address(lpm.subscriber(tokenId)), address(0)); } + function test_unsubscribe_selfDestructed() public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, alice, ZERO_BYTES); + + // approve this contract to operate on alices liq + vm.startPrank(alice); + lpm.approve(address(this), tokenId); + vm.stopPrank(); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + + // simulate selfdestruct by etching the bytecode to 0 + vm.etch(address(sub), ZERO_BYTES); + + lpm.unsubscribe(tokenId, config); + + assertEq(lpm.hasSubscriber(tokenId), false); + assertEq(address(lpm.subscriber(tokenId)), address(0)); + } + function test_multicall_mint_subscribe() public { uint256 tokenId = lpm.nextTokenId(); @@ -339,7 +437,24 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { lpm.approve(address(this), tokenId); vm.stopPrank(); - vm.expectRevert(); + vm.expectRevert(INotifier.NotSubscribed.selector); + lpm.unsubscribe(tokenId, config); + } + + function test_unsubscribe_twice_reverts() public { + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, alice, ZERO_BYTES); + + // approve this contract to operate on alices liq + vm.startPrank(alice); + lpm.approve(address(this), tokenId); + vm.stopPrank(); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + + lpm.unsubscribe(tokenId, config); + + vm.expectRevert(INotifier.NotSubscribed.selector); lpm.unsubscribe(tokenId, config); } @@ -500,4 +615,31 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot { assertEq(lpm.hasSubscriber(tokenId), false); assertEq(sub.notifyUnsubscribeCount(), 1); } + + /// @notice Test that users cannot forcibly avoid unsubscribe logic via gas limits + function test_fuzz_unsubscribe_with_gas_limit(uint64 gasLimit) public { + // enforce a minimum amount of gas to avoid OutOfGas reverts + gasLimit = uint64(bound(gasLimit, 125_000, block.gaslimit)); + + uint256 tokenId = lpm.nextTokenId(); + mint(config, 100e18, alice, ZERO_BYTES); + + // approve this contract to operate on alices liq + vm.startPrank(alice); + lpm.approve(address(this), tokenId); + vm.stopPrank(); + + lpm.subscribe(tokenId, config, address(sub), ZERO_BYTES); + uint256 beforeUnsubCount = sub.notifyUnsubscribeCount(); + + if (gasLimit < lpm.unsubscribeGasLimit()) { + // gas too low to call a valid unsubscribe + vm.expectRevert(INotifier.GasLimitTooLow.selector); + lpm.unsubscribe{gas: gasLimit}(tokenId, config); + } else { + // increasing gas limit succeeds and unsubscribe was called + lpm.unsubscribe{gas: gasLimit}(tokenId, config); + assertEq(sub.notifyUnsubscribeCount(), beforeUnsubCount + 1); + } + } } diff --git a/test/shared/PosmTestSetup.sol b/test/shared/PosmTestSetup.sol index 5c35c316f..81c4f9bf5 100644 --- a/test/shared/PosmTestSetup.sol +++ b/test/shared/PosmTestSetup.sol @@ -57,7 +57,7 @@ contract PosmTestSetup is Test, Deployers, DeployPermit2, LiquidityOperations { function deployPosm(IPoolManager poolManager) internal { // We use deployPermit2() to prevent having to use via-ir in this repository. permit2 = IAllowanceTransfer(deployPermit2()); - lpm = new PositionManager(poolManager, permit2); + lpm = new PositionManager(poolManager, permit2, 100_000); } function seedBalance(address to) internal {