OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, Unsubscribe Minimum Gas Limit #318

saucepoint · 2024-08-30T00:23:51Z

Related Issue

Which issue does this pull request resolve?

OZ-L10: calls to addresses without bytecode will still hard-revert, even with try/catch

For chains that still support selfdestruct, you can subscribe to a valid contract but then lose access to unsubscribing if the contract self-destructs

SB L53: Notifications call contracts without code

SB L54: Subscribing multiple times to contracts without code

SB M78: Unsubscribe should have a Minimum Gas Limit

Description of changes

if subscriber has no code then unsubscribe will not revert
Added a custom error for unsubscribing when already unsubscribed
if subscriber has no code then notifying subscribe, transfer, and modifyLiquidity will revert
Unsubscribe requires a minimum gas limit otherwise revert. before users can use pick a gas limit where notifyUnsubscribe reverts (and is try/catched) with OutOfGas but the overall transaction still succeeds

saucepoint · 2024-08-30T21:05:42Z

src/base/Notifier.sol

+            // 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
+            // to account for EIP-150, condition could be 64 * gasleft() / 63 <= subscriberGasLimit
+            if ((64 * gasleft() / 63) < subscriberGasLimit) GasLimitTooLow.selector.revertWith();
+            try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config, data) {} catch {}


need extra eyes on this (and the natspec)

feels weirdly unintuitive that users may need to specify a very high gas limit on .unsubscribe() calls

@snreynolds @hensha256

yeah it means that all users will have to send in gasLimit > subscriberGasLimit, even if their particular subsciber only needs like 10k in gas. Which sucks really... but I can't see any other way around it?
I think it also means that it will be really hard to run estimateGas because of this require statement.

hensha256 · 2024-09-02T19:28:03Z

src/base/Notifier.sol

+            // otherwise, users can select a gas limit where .notifyUnsubscribe hits OutOfGas yet the transaction/unsubscription
+            // can still succeed
+            // to account for EIP-150, condition could be 64 * gasleft() / 63 <= subscriberGasLimit
+            if ((64 * gasleft() / 63) < subscriberGasLimit) GasLimitTooLow.selector.revertWith();


in core we dont bother with the *64/63. Theres just a comment about it
https://github.com/Uniswap/v4-core/blob/4dc48bbd89f4faf1e9493cd1563908d79aa10b0d/src/ProtocolFees.sol#L76

thoughts?

src/interfaces/INotifier.sol

Co-authored-by: Alice <[email protected]>

hensha256 · 2024-09-04T01:21:21Z

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()


Suggested change

/// @dev Because of EIP-150, solidity may only allocate 63/64 of gasleft()

/// @dev Because of EIP-150, solidity may only allocate 63/64 of unsubscribeGasLimit

i actually think its clearer to leave it as

/// @dev Because of EIP-150, solidity may only allocate 63/64 of gasleft()

because this only happens when gasLeft == unsubscriberGasLimit

i just mean that subscribers should know that they actually cannot take more than 63/64 of unsubscribeGasLimit because thats the lower bound of gas they will receive

yeah agree that is the lower bound !

hensha256 · 2024-09-04T01:34:02Z

src/base/Notifier.sol

@@ -65,13 +66,17 @@ abstract contract Notifier is INotifier {
    function _unsubscribe(uint256 tokenId, PositionConfig calldata config) internal {
        _positionConfigs(tokenId).setUnsubscribe();
        ISubscriber _subscriber = subscriber[tokenId];
+        if (_subscriber == NO_SUBSCRIBER) NotSubscribed.selector.revertWith();


i see that this check makes sense, surprised it wasnt here before
doesnt one of these issues say to add it? i cant find it

I think it may make more sense to check _postionConfigs().hasSubscriber? i guess see if its cheaper in the case where they do have a subscriber?

yep looked like .hasSubscriber() was slightly cheaper! refactored it

src/base/Notifier.sol

snreynolds · 2024-09-04T03:41:39Z

src/base/Notifier.sol

-            if ((64 * gasleft() / 63) < subscriberGasLimit) GasLimitTooLow.selector.revertWith();
-            try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config) {} catch {}
+            // to account for EIP-150, condition could be 64 * gasleft() / 63 <= unsubscribeGasLimit
+            if ((64 * gasleft() / 63) < unsubscribeGasLimit) GasLimitTooLow.selector.revertWith();


I think we do not need to calculate the (64 * gasLeft / 63) check.

Really this line is just being added to enforce some minimum amount that a user MUST send.

That minimum amount doesn't exactly need to be 63/64ths of the unsubscribeGasLimit. It can just be at least the unsubscribeGasLimit. And we can add a comment that in 1 case (when gasLeft == gasLimit) the call will only forward 63/64ths of the gasLimit... which is fine. Basically we can just keep this in mind when parameterizing the unsubscribeGasLimit.

We really want to add this line because we do not want the user to ever send a minimal amount of gas such that the unsubscribe block reverts but the rest of the function succeeds

snreynolds

I see that we changed it to document EIP150. I just left some comments in response to Alice's about the natspec!

Looks good!

snreynolds · 2024-09-04T03:43:44Z

src/base/Notifier.sol

-    // 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


nit: i think there is an extra line added that can be removed.

this natspec? fwiw i added this to INotifier

/// @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);

snreynolds · 2024-09-04T03:47:07Z

src/base/Notifier.sol

@@ -65,13 +66,17 @@ abstract contract Notifier is INotifier {
    function _unsubscribe(uint256 tokenId, PositionConfig calldata config) internal {
        _positionConfigs(tokenId).setUnsubscribe();
        ISubscriber _subscriber = subscriber[tokenId];
+        if (_subscriber == NO_SUBSCRIBER) NotSubscribed.selector.revertWith();


I think it may make more sense to check _postionConfigs().hasSubscriber? i guess see if its cheaper in the case where they do have a subscriber?

snreynolds · 2024-09-04T03:48:34Z

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()


i actually think its clearer to leave it as

/// @dev Because of EIP-150, solidity may only allocate 63/64 of gasleft()

because this only happens when gasLeft == unsubscriberGasLimit

Co-authored-by: Alice <[email protected]>

snreynolds

nice

saucepoint added 2 commits August 29, 2024 20:15

check subscriber code length

eca398f

revert unsubscribe if not subscribed

c93fc95

saucepoint requested review from snreynolds and hensha256 August 30, 2024 00:25

check if subscriber has code before notifying

dd948d2

saucepoint changed the title ~~OZ-L10: Check code length on Unsubscribe~~ OZ-L10: Check code length on Notifications Aug 30, 2024

custom reverts

845a1d4

hensha256 previously approved these changes Aug 30, 2024

View reviewed changes

saucepoint and others added 2 commits August 30, 2024 15:43

Merge branch 'main' into oz-L10-unsubscribe-revert

7710b38

SB-M78: minimum gas limit on unsubscribe

732bee4

saucepoint dismissed hensha256’s stale review via 732bee4 August 30, 2024 20:35

saucepoint changed the title ~~OZ-L10: Check code length on Notifications~~ OZ-L10, SB-78; Check code length on Notifications, Unsubscribe Minimum Gas Limit Aug 30, 2024

saucepoint changed the title ~~OZ-L10, SB-78; Check code length on Notifications, Unsubscribe Minimum Gas Limit~~ OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, Unsubscribe Minimum Gas Limit Aug 30, 2024

saucepoint commented Aug 30, 2024

View reviewed changes

natspec on unsubscribe for SB M78

234596f

hensha256 reviewed Sep 2, 2024

View reviewed changes

saucepoint and others added 7 commits September 3, 2024 10:45

Update src/interfaces/INotifier.sol

a05c0c9

Co-authored-by: Alice <[email protected]>

merge in main; fix conflicts

12989e5

use constructor for subscriber gas limit

e7fe727

natspec for unsubscribeGasLimit

de32571

document EIP-150 instead

e96d8d3

merge in main; regenerate gas; forge fmt

df2be3b

merge in main; regenerate gas

f23b137

hensha256 reviewed Sep 4, 2024

View reviewed changes

src/base/Notifier.sol Outdated Show resolved Hide resolved

snreynolds reviewed Sep 4, 2024

View reviewed changes

saucepoint and others added 2 commits September 4, 2024 11:08

Update src/base/Notifier.sol

0faaa59

Co-authored-by: Alice <[email protected]>

forge fmt lol

21fa51f

saucepoint added 2 commits September 4, 2024 11:33

gas benchmarks for subscribe/unsubscribe

f151630

gas optimize unsubscribe check

dc3a623

snreynolds previously approved these changes Sep 4, 2024

View reviewed changes

merge in main; fix to account for CEI

6caa7cc

saucepoint dismissed snreynolds’s stale review via 6caa7cc September 4, 2024 18:20

snreynolds approved these changes Sep 4, 2024

View reviewed changes

hensha256 approved these changes Sep 4, 2024

View reviewed changes

hensha256 merged commit 255b20e into main Sep 4, 2024
3 checks passed

hensha256 deleted the oz-L10-unsubscribe-revert branch September 4, 2024 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, Unsubscribe Minimum Gas Limit #318

OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, Unsubscribe Minimum Gas Limit #318

saucepoint commented Aug 30, 2024 •

edited

Loading

saucepoint Aug 30, 2024

hensha256 Sep 2, 2024

hensha256 Sep 2, 2024

snreynolds Sep 3, 2024

hensha256 Sep 4, 2024

snreynolds Sep 4, 2024

hensha256 Sep 4, 2024

snreynolds Sep 4, 2024

hensha256 Sep 4, 2024

snreynolds Sep 4, 2024

saucepoint Sep 4, 2024

snreynolds Sep 4, 2024

snreynolds left a comment

snreynolds Sep 4, 2024

saucepoint Sep 4, 2024

snreynolds Sep 4, 2024

snreynolds Sep 4, 2024

snreynolds left a comment

	/// @dev Because of EIP-150, solidity may only allocate 63/64 of gasleft()
	/// @dev Because of EIP-150, solidity may only allocate 63/64 of unsubscribeGasLimit

OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, Unsubscribe Minimum Gas Limit #318

OZ-L10, SB-L53, SB-L54, SB-M78; Check code length on Notifications, Unsubscribe Minimum Gas Limit #318

Conversation

saucepoint commented Aug 30, 2024 • edited Loading

Related Issue

Description of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snreynolds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snreynolds left a comment

Choose a reason for hiding this comment

saucepoint commented Aug 30, 2024 •

edited

Loading