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 5 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
10 changes: 7 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,14 @@ 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);
try _subscriber.notifyUnsubscribe{gas: subscriberGasLimit}(tokenId, config, data) {} catch {}
}

emit Unsubscribed(tokenId, address(_subscriber));
}
Expand Down Expand Up @@ -106,6 +109,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
4 changes: 4 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,10 @@ 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 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
100 changes: 99 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 @@ -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_notifyModifyLiquidity_succeeds() public {
uint256 tokenId = lpm.nextTokenId();
mint(config, 100e18, alice, ZERO_BYTES);
Expand Down Expand Up @@ -126,6 +142,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 +211,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 +250,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 +327,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 +418,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
Loading