Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 19 commits into from
Sep 4, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
eca398f
check subscriber code length
saucepoint Aug 30, 2024
c93fc95
revert unsubscribe if not subscribed
saucepoint Aug 30, 2024
dd948d2
check if subscriber has code before notifying
saucepoint Aug 30, 2024
845a1d4
custom reverts
saucepoint Aug 30, 2024
7710b38
Merge branch 'main' into oz-L10-unsubscribe-revert
saucepoint Aug 30, 2024
732bee4
SB-M78: minimum gas limit on unsubscribe
saucepoint Aug 30, 2024
234596f
natspec on unsubscribe for SB M78
saucepoint Aug 30, 2024
a05c0c9
Update src/interfaces/INotifier.sol
saucepoint Sep 3, 2024
12989e5
merge in main; fix conflicts
saucepoint Sep 3, 2024
e7fe727
use constructor for subscriber gas limit
saucepoint Sep 3, 2024
de32571
natspec for unsubscribeGasLimit
saucepoint Sep 3, 2024
e96d8d3
document EIP-150 instead
saucepoint Sep 3, 2024
df2be3b
merge in main; regenerate gas; forge fmt
saucepoint Sep 3, 2024
f23b137
merge in main; regenerate gas
saucepoint Sep 3, 2024
0faaa59
Update src/base/Notifier.sol
saucepoint Sep 4, 2024
21fa51f
forge fmt lol
saucepoint Sep 4, 2024
f151630
gas benchmarks for subscribe/unsubscribe
saucepoint Sep 4, 2024
dc3a623
gas optimize unsubscribe check
saucepoint Sep 4, 2024
6caa7cc
merge in main; fix to account for CEI
saucepoint Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/base/Notifier.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ abstract contract Notifier is INotifier {
_positionConfigs(tokenId).setSubscribe();
ISubscriber _subscriber = subscriber[tokenId];

if (_subscriber != NO_SUBSCRIBER) revert AlreadySubscribed(address(_subscriber));
if (_subscriber != NO_SUBSCRIBER) AlreadySubscribed.selector.revertWith(address(_subscriber));
subscriber[tokenId] = ISubscriber(newSubscriber);

bool success = _call(newSubscriber, abi.encodeCall(ISubscriber.notifySubscribe, (tokenId, config, data)));
Expand All @@ -67,11 +67,20 @@ abstract contract Notifier is INotifier {
{
_positionConfigs(tokenId).setUnsubscribe();
ISubscriber _subscriber = subscriber[tokenId];
if (_subscriber == NO_SUBSCRIBER) NotSubscribed.selector.revertWith();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


delete subscriber[tokenId];

uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS);
try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config, data) {} catch {}
if (address(_subscriber).code.length > 0) {
uint256 subscriberGasLimit = block.gaslimit.calculatePortion(BLOCK_LIMIT_BPS);

// 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
saucepoint marked this conversation as resolved.
Show resolved Hide resolved
// to account for EIP-150, condition could be 64 * gasleft() / 63 <= subscriberGasLimit
if ((64 * gasleft() / 63) < subscriberGasLimit) GasLimitTooLow.selector.revertWith();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config, data) {} catch {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

emit Unsubscribed(tokenId, address(_subscriber));
}
Expand Down Expand Up @@ -106,6 +115,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)
}
Expand Down
7 changes: 7 additions & 0 deletions src/interfaces/INotifier.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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__SubsciptionReverted(address subscriber, bytes reason);
/// @notice Wraps the revert message of the subscriber contract on a reverting modify liquidity notification
Expand Down Expand Up @@ -33,6 +39,7 @@ interface INotifier {
/// @param tokenId the ERC721 tokenId
/// @param config the corresponding PositionConfig for the tokenId
/// @param data caller-provided data that's forwarded to the subscriber contract
/// @dev Callers must specify a high gas limit (remaining gas should be higher than 300,000 gas) such that the subscriber can be notified
saucepoint marked this conversation as resolved.
Show resolved Hide resolved
/// @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, bytes calldata data) external payable;
Expand Down
131 changes: 130 additions & 1 deletion test/position-managers/PositionManager.notifier.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@ import {Actions} from "../../src/libraries/Actions.sol";
import {INotifier} from "../../src/interfaces/INotifier.sol";
import {MockReturnDataSubscriber, MockRevertSubscriber} from "../mocks/MockBadSubscribers.sol";
import {BalanceDelta, toBalanceDelta} from "@uniswap/v4-core/src/types/BalanceDelta.sol";
import {BipsLibrary} from "../../src/libraries/BipsLibrary.sol";

contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
using PoolIdLibrary for PoolKey;
using StateLibrary for IPoolManager;
using Planner for Plan;
using BipsLibrary for uint256;

MockSubscriber sub;
MockReturnDataSubscriber badSubscriber;
Expand Down Expand Up @@ -97,6 +99,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_notifyModifyLiquidity_succeeds() public {
uint256 tokenId = lpm.nextTokenId();
mint(config, 100e18, alice, ZERO_BYTES);
Expand Down Expand Up @@ -126,6 +144,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);
Expand Down Expand Up @@ -173,6 +213,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);
Expand All @@ -192,6 +252,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);
Expand Down Expand Up @@ -249,6 +329,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, ZERO_BYTES);

assertEq(lpm.hasSubscriber(tokenId), false);
assertEq(address(lpm.subscriber(tokenId)), address(0));
}

function test_multicall_mint_subscribe() public {
uint256 tokenId = lpm.nextTokenId();

Expand Down Expand Up @@ -320,7 +420,7 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
lpm.approve(address(this), tokenId);
vm.stopPrank();

vm.expectRevert();
vm.expectRevert(INotifier.NotSubscribed.selector);
lpm.unsubscribe(tokenId, config, ZERO_BYTES);
}

Expand Down Expand Up @@ -477,4 +577,33 @@ contract PositionManagerNotifierTest is Test, PosmTestSetup, GasSnapshot {
);
lpm.safeTransferFrom(alice, bob, tokenId, "");
}

/// @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, 150_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();

uint256 subscriberGasLimit = block.gaslimit.calculatePortion(100);

if (gasLimit < subscriberGasLimit) {
// gas too low to call a valid unsubscribe
vm.expectRevert(INotifier.GasLimitTooLow.selector);
lpm.unsubscribe{gas: gasLimit}(tokenId, config, ZERO_BYTES);
} else {
// increasing gas limit succeeds and unsubscribe was called
lpm.unsubscribe{gas: gasLimit}(tokenId, config, ZERO_BYTES);
assertEq(sub.notifyUnsubscribeCount(), beforeUnsubCount + 1);
}
}
}
Loading