Remove the ability to cancel for recipients (#710)

* feat: remove ability to cancel for recipient feat: remove sender's hook test: update tests accordingly * feat: emit asset in cancel and withdraw events refactor: remove recipient from cancel event test: update tests accordingly test: update precompiles bytecode * chore: update gas snapshot * test: update Precompiles bytecode * docs: include recipient disable to cancel in changelog * refactor: add "recipient" in cancel event docs: refine explanatory comments refactor: reorder parameters in events * test: update Precompiles bytecode * chore: add explantory comment --------- Co-authored-by: Paul Razvan Berg <[email protected]>
sablier-labs · Dec 15, 2023 · 0b9b3e1 · 0b9b3e1
1 parent bc4be5b
commit 0b9b3e1
Show file tree

Hide file tree

Showing 36 changed files with 502 additions and 961 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,7 @@ The format is based on [Common Changelog](https://common-changelog.org/).
 
 ### Changed
 
+- **Breaking**: Remove ability to `cancel` for recipients ([#710](https://github.com/sablier-labs/v2-core/pull/710))
 - Replace the streamed amount with the deposit amount in the NFT descriptor
   ([#692](https://github.com/sablier-labs/v2-core/pull/692))
 - Upgrade Solidity to `0.8.21` ([#688](https://github.com/sablier-labs/v2-core/pull/688))

diff --git a/shell/prepare-artifacts.sh b/shell/prepare-artifacts.sh
@@ -45,7 +45,6 @@ cp out-optimized/IERC721Metadata.sol/IERC721Metadata.json $erc721
 
 hooks=./artifacts/interfaces/hooks
 cp out-optimized/ISablierV2LockupRecipient.sol/ISablierV2LockupRecipient.json $hooks
-cp out-optimized/ISablierV2LockupSender.sol/ISablierV2LockupSender.json $hooks
 
 libraries=./artifacts/libraries
 cp out-optimized/Errors.sol/Errors.json $libraries

diff --git a/src/SablierV2LockupDynamic.sol b/src/SablierV2LockupDynamic.sol
@@ -14,7 +14,6 @@ import { ISablierV2Comptroller } from "./interfaces/ISablierV2Comptroller.sol";
 import { ISablierV2Lockup } from "./interfaces/ISablierV2Lockup.sol";
 import { ISablierV2LockupDynamic } from "./interfaces/ISablierV2LockupDynamic.sol";
 import { ISablierV2LockupRecipient } from "./interfaces/hooks/ISablierV2LockupRecipient.sol";
-import { ISablierV2LockupSender } from "./interfaces/hooks/ISablierV2LockupSender.sol";
 import { ISablierV2NFTDescriptor } from "./interfaces/ISablierV2NFTDescriptor.sol";
 import { Errors } from "./libraries/Errors.sol";
 import { Helpers } from "./libraries/Helpers.sol";
@@ -507,38 +506,25 @@ contract SablierV2LockupDynamic is
         address sender = _streams[streamId].sender;
         address recipient = _ownerOf(streamId);
 
+        // Retrieve the ERC-20 asset from storage.
+        IERC20 asset = _streams[streamId].asset;
+
         // Interactions: refund the sender.
-        _streams[streamId].asset.safeTransfer({ to: sender, value: senderAmount });
-
-        // Interactions: if `msg.sender` is the sender and the recipient is a contract, try to invoke the cancel
-        // hook on the recipient without reverting if the hook is not implemented, and without bubbling up any
-        // potential revert.
-        if (msg.sender == sender) {
-            if (recipient.code.length > 0) {
-                try ISablierV2LockupRecipient(recipient).onStreamCanceled({
-                    streamId: streamId,
-                    sender: sender,
-                    senderAmount: senderAmount,
-                    recipientAmount: recipientAmount
-                }) { } catch { }
-            }
-        }
-        // Interactions: if `msg.sender` is the recipient and the sender is a contract, try to invoke the cancel
-        // hook on the sender without reverting if the hook is not implemented, and also without bubbling up any
-        // potential revert.
-        else {
-            if (sender.code.length > 0) {
-                try ISablierV2LockupSender(sender).onStreamCanceled({
-                    streamId: streamId,
-                    recipient: recipient,
-                    senderAmount: senderAmount,
-                    recipientAmount: recipientAmount
-                }) { } catch { }
-            }
+        asset.safeTransfer({ to: sender, value: senderAmount });
+
+        // Interactions: if the recipient is a contract, try to invoke the cancel hook on the recipient without
+        // reverting if the hook is not implemented, and without bubbling up any potential revert.
+        if (recipient.code.length > 0) {
+            try ISablierV2LockupRecipient(recipient).onStreamCanceled({
+                streamId: streamId,
+                sender: sender,
+                senderAmount: senderAmount,
+                recipientAmount: recipientAmount
+            }) { } catch { }
         }
 
         // Log the cancellation.
-        emit ISablierV2Lockup.CancelLockupStream(streamId, sender, recipient, senderAmount, recipientAmount);
+        emit ISablierV2Lockup.CancelLockupStream(streamId, sender, recipient, asset, senderAmount, recipientAmount);
     }
 
     /// @dev See the documentation for the user-facing functions that call this internal function.
@@ -650,7 +636,13 @@ contract SablierV2LockupDynamic is
             _streams[streamId].isCancelable = false;
         }
 
+        // Retrieve the ERC-20 asset from storage.
+        IERC20 asset = _streams[streamId].asset;
+
         // Interactions: perform the ERC-20 transfer.
-        _streams[streamId].asset.safeTransfer({ to: to, value: amount });
+        asset.safeTransfer({ to: to, value: amount });
+
+        // Log the withdrawal.
+        emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, asset, amount);
     }
 }
diff --git a/src/SablierV2LockupLinear.sol b/src/SablierV2LockupLinear.sol
@@ -12,7 +12,6 @@ import { ISablierV2Lockup } from "./interfaces/ISablierV2Lockup.sol";
 import { ISablierV2LockupLinear } from "./interfaces/ISablierV2LockupLinear.sol";
 import { ISablierV2NFTDescriptor } from "./interfaces/ISablierV2NFTDescriptor.sol";
 import { ISablierV2LockupRecipient } from "./interfaces/hooks/ISablierV2LockupRecipient.sol";
-import { ISablierV2LockupSender } from "./interfaces/hooks/ISablierV2LockupSender.sol";
 import { Errors } from "./libraries/Errors.sol";
 import { Helpers } from "./libraries/Helpers.sol";
 import { Lockup, LockupLinear } from "./types/DataTypes.sol";
@@ -422,38 +421,25 @@ contract SablierV2LockupLinear is
         address sender = _streams[streamId].sender;
         address recipient = _ownerOf(streamId);
 
+        // Retrieve the ERC-20 asset from storage.
+        IERC20 asset = _streams[streamId].asset;
+
         // Interactions: refund the sender.
-        _streams[streamId].asset.safeTransfer({ to: sender, value: senderAmount });
-
-        // Interactions: if `msg.sender` is the sender and the recipient is a contract, try to invoke the cancel
-        // hook on the recipient without reverting if the hook is not implemented, and without bubbling up any
-        // potential revert.
-        if (msg.sender == sender) {
-            if (recipient.code.length > 0) {
-                try ISablierV2LockupRecipient(recipient).onStreamCanceled({
-                    streamId: streamId,
-                    sender: sender,
-                    senderAmount: senderAmount,
-                    recipientAmount: recipientAmount
-                }) { } catch { }
-            }
-        }
-        // Interactions: if `msg.sender` is the recipient and the sender is a contract, try to invoke the cancel
-        // hook on the sender without reverting if the hook is not implemented, and also without bubbling up any
-        // potential revert.
-        else {
-            if (sender.code.length > 0) {
-                try ISablierV2LockupSender(sender).onStreamCanceled({
-                    streamId: streamId,
-                    recipient: recipient,
-                    senderAmount: senderAmount,
-                    recipientAmount: recipientAmount
-                }) { } catch { }
-            }
+        asset.safeTransfer({ to: sender, value: senderAmount });
+
+        // Interactions: if the recipient is a contract, try to invoke the cancel hook on the recipient without
+        // reverting if the hook is not implemented, and without bubbling up any potential revert.
+        if (recipient.code.length > 0) {
+            try ISablierV2LockupRecipient(recipient).onStreamCanceled({
+                streamId: streamId,
+                sender: sender,
+                senderAmount: senderAmount,
+                recipientAmount: recipientAmount
+            }) { } catch { }
         }
 
         // Log the cancellation.
-        emit ISablierV2Lockup.CancelLockupStream(streamId, sender, recipient, senderAmount, recipientAmount);
+        emit ISablierV2Lockup.CancelLockupStream(streamId, sender, recipient, asset, senderAmount, recipientAmount);
     }
 
     /// @dev See the documentation for the user-facing functions that call this internal function.
@@ -556,7 +542,13 @@ contract SablierV2LockupLinear is
             _streams[streamId].isCancelable = false;
         }
 
+        // Retrieve the ERC-20 asset from storage.
+        IERC20 asset = _streams[streamId].asset;
+
         // Interactions: perform the ERC-20 transfer.
-        _streams[streamId].asset.safeTransfer({ to: to, value: amount });
+        asset.safeTransfer({ to: to, value: amount });
+
+        // Log the withdrawal.
+        emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, asset, amount);
     }
 }
diff --git a/src/abstracts/SablierV2Lockup.sol b/src/abstracts/SablierV2Lockup.sol
@@ -158,8 +158,8 @@ abstract contract SablierV2Lockup is
             revert Errors.SablierV2Lockup_StreamCanceled(streamId);
         }
 
-        // Checks: `msg.sender` is either the stream's sender or the stream's recipient (i.e. the NFT owner).
-        if (!_isCallerStreamSender(streamId) && msg.sender != _ownerOf(streamId)) {
+        // Checks: `msg.sender` is the stream's sender.
+        if (!_isCallerStreamSender(streamId)) {
             revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
         }
 
@@ -291,9 +291,6 @@ abstract contract SablierV2Lockup is
                 amount: amount
             }) { } catch { }
         }
-
-        // Log the withdrawal.
-        emit ISablierV2Lockup.WithdrawFromLockupStream(streamId, to, amount);
     }
 
     /// @inheritdoc ISablierV2Lockup

diff --git a/src/interfaces/ISablierV2Lockup.sol b/src/interfaces/ISablierV2Lockup.sol
@@ -22,14 +22,16 @@ interface ISablierV2Lockup is
     /// @param streamId The id of the stream.
     /// @param sender The address of the stream's sender.
     /// @param recipient The address of the stream's recipient.
+    /// @param asset The contract address of the ERC-20 asset used for streaming.
     /// @param senderAmount The amount of assets refunded to the stream's sender, denoted in units of the asset's
     /// decimals.
     /// @param recipientAmount The amount of assets left for the stream's recipient to withdraw, denoted in units of the
     /// asset's decimals.
     event CancelLockupStream(
-        uint256 indexed streamId,
+        uint256 streamId,
         address indexed sender,
         address indexed recipient,
+        IERC20 indexed asset,
         uint128 senderAmount,
         uint128 recipientAmount
     );
@@ -49,8 +51,9 @@ interface ISablierV2Lockup is
     /// @notice Emitted when assets are withdrawn from a stream.
     /// @param streamId The id of the stream.
     /// @param to The address that has received the withdrawn assets.
+    /// @param asset The contract address of the ERC-20 asset used for streaming.
     /// @param amount The amount of assets withdrawn, denoted in units of the asset's decimals.
-    event WithdrawFromLockupStream(uint256 indexed streamId, address indexed to, uint128 amount);
+    event WithdrawFromLockupStream(uint256 indexed streamId, address indexed to, IERC20 indexed asset, uint128 amount);
 
     /*//////////////////////////////////////////////////////////////////////////
                                  CONSTANT FUNCTIONS
@@ -187,7 +190,7 @@ interface ISablierV2Lockup is
     /// Requirements:
     /// - Must not be delegate called.
     /// - The stream must be warm and cancelable.
-    /// - `msg.sender` must be either the stream's sender or the stream's recipient (i.e. the NFT owner).
+    /// - `msg.sender` must be the stream's sender.
     ///
     /// @param streamId The id of the stream to cancel.
     function cancel(uint256 streamId) external;

diff --git a/src/interfaces/hooks/ISablierV2LockupSender.sol b/src/interfaces/hooks/ISablierV2LockupSender.sol
diff --git a/test/Base.t.sol b/test/Base.t.sol
@@ -18,7 +18,6 @@ import { ERC20MissingReturn } from "./mocks/erc20/ERC20MissingReturn.sol";
 import { GoodFlashLoanReceiver } from "./mocks/flash-loan/GoodFlashLoanReceiver.sol";
 import { Noop } from "./mocks/Noop.sol";
 import { GoodRecipient } from "./mocks/hooks/GoodRecipient.sol";
-import { GoodSender } from "./mocks/hooks/GoodSender.sol";
 import { Assertions } from "./utils/Assertions.sol";
 import { Calculations } from "./utils/Calculations.sol";
 import { Constants } from "./utils/Constants.sol";
@@ -44,7 +43,6 @@ abstract contract Base_Test is Assertions, Calculations, Constants, Events, Fuzz
     Defaults internal defaults;
     GoodFlashLoanReceiver internal goodFlashLoanReceiver;
     GoodRecipient internal goodRecipient;
-    GoodSender internal goodSender;
     ISablierV2LockupDynamic internal lockupDynamic;
     ISablierV2LockupLinear internal lockupLinear;
     ISablierV2NFTDescriptor internal nftDescriptor;
@@ -60,15 +58,13 @@ abstract contract Base_Test is Assertions, Calculations, Constants, Events, Fuzz
         dai = new ERC20("Dai Stablecoin", "DAI");
         goodFlashLoanReceiver = new GoodFlashLoanReceiver();
         goodRecipient = new GoodRecipient();
-        goodSender = new GoodSender();
         noop = new Noop();
         usdt = new ERC20MissingReturn("Tether USD", "USDT", 6);
 
         // Label the base test contracts.
         vm.label({ account: address(dai), newLabel: "DAI" });
         vm.label({ account: address(goodFlashLoanReceiver), newLabel: "Good Flash Loan Receiver" });
         vm.label({ account: address(goodRecipient), newLabel: "Good Recipient" });
-        vm.label({ account: address(goodSender), newLabel: "Good Sender" });
         vm.label({ account: address(nftDescriptor), newLabel: "NFT Descriptor" });
         vm.label({ account: address(noop), newLabel: "Noop" });
         vm.label({ account: address(usdt), newLabel: "USDT" });

diff --git a/test/fork/LockupDynamic.t.sol b/test/fork/LockupDynamic.t.sol
@@ -288,6 +288,7 @@ abstract contract LockupDynamic_Fork_Test is Fork_Test {
             emit WithdrawFromLockupStream({
                 streamId: vars.streamId,
                 to: params.recipient,
+                asset: ASSET,
                 amount: params.withdrawAmount
             });
             vm.expectEmit({ emitter: address(lockupDynamic) });
@@ -351,7 +352,7 @@ abstract contract LockupDynamic_Fork_Test is Fork_Test {
             vars.senderAmount = lockupDynamic.refundableAmountOf(vars.streamId);
             vars.recipientAmount = lockupDynamic.withdrawableAmountOf(vars.streamId);
             emit CancelLockupStream(
-                vars.streamId, params.sender, params.recipient, vars.senderAmount, vars.recipientAmount
+                vars.streamId, params.sender, params.recipient, ASSET, vars.senderAmount, vars.recipientAmount
             );
             vm.expectEmit({ emitter: address(lockupDynamic) });
             emit MetadataUpdate({ _tokenId: vars.streamId });

diff --git a/test/fork/LockupLinear.t.sol b/test/fork/LockupLinear.t.sol
@@ -274,6 +274,7 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
             emit WithdrawFromLockupStream({
                 streamId: vars.streamId,
                 to: params.recipient,
+                asset: ASSET,
                 amount: params.withdrawAmount
             });
             vm.expectEmit({ emitter: address(lockupLinear) });
@@ -335,7 +336,7 @@ abstract contract LockupLinear_Fork_Test is Fork_Test {
             vars.senderAmount = lockupLinear.refundableAmountOf(vars.streamId);
             vars.recipientAmount = lockupLinear.withdrawableAmountOf(vars.streamId);
             emit CancelLockupStream(
-                vars.streamId, params.sender, params.recipient, vars.senderAmount, vars.recipientAmount
+                vars.streamId, params.sender, params.recipient, ASSET, vars.senderAmount, vars.recipientAmount
             );
             vm.expectEmit({ emitter: address(lockupLinear) });
             emit MetadataUpdate({ _tokenId: vars.streamId });

diff --git a/test/integration/Integration.t.sol b/test/integration/Integration.t.sol
@@ -7,9 +7,7 @@ import { Base_Test } from "../Base.t.sol";
 import { FaultyFlashLoanReceiver } from "../mocks/flash-loan/FaultyFlashLoanReceiver.sol";
 import { ReentrantFlashLoanReceiver } from "../mocks/flash-loan/ReentrantFlashLoanReceiver.sol";
 import { ReentrantRecipient } from "../mocks/hooks/ReentrantRecipient.sol";
-import { ReentrantSender } from "../mocks/hooks/ReentrantSender.sol";
 import { RevertingRecipient } from "../mocks/hooks/RevertingRecipient.sol";
-import { RevertingSender } from "../mocks/hooks/RevertingSender.sol";
 
 /// @notice Common logic needed by all integration tests, both concrete and fuzz tests.
 abstract contract Integration_Test is Base_Test {
@@ -20,9 +18,7 @@ abstract contract Integration_Test is Base_Test {
     FaultyFlashLoanReceiver internal faultyFlashLoanReceiver = new FaultyFlashLoanReceiver();
     ReentrantFlashLoanReceiver internal reentrantFlashLoanReceiver = new ReentrantFlashLoanReceiver();
     ReentrantRecipient internal reentrantRecipient = new ReentrantRecipient();
-    ReentrantSender internal reentrantSender = new ReentrantSender();
     RevertingRecipient internal revertingRecipient = new RevertingRecipient();
-    RevertingSender internal revertingSender = new RevertingSender();
 
     /*//////////////////////////////////////////////////////////////////////////
                                   SET-UP FUNCTION
@@ -53,9 +49,7 @@ abstract contract Integration_Test is Base_Test {
         vm.label({ account: address(faultyFlashLoanReceiver), newLabel: "Faulty Flash Loan Receiver" });
         vm.label({ account: address(reentrantFlashLoanReceiver), newLabel: "Reentrant Flash Loan Receiver" });
         vm.label({ account: address(reentrantRecipient), newLabel: "Reentrant Lockup Recipient" });
-        vm.label({ account: address(reentrantSender), newLabel: "Reentrant Lockup Sender" });
         vm.label({ account: address(revertingRecipient), newLabel: "Reverting Lockup Recipient" });
-        vm.label({ account: address(revertingSender), newLabel: "Reverting Lockup Sender" });
     }
 
     /*//////////////////////////////////////////////////////////////////////////

diff --git a/test/integration/concrete/lockup/burn/burn.t.sol b/test/integration/concrete/lockup/burn/burn.t.sol
@@ -83,7 +83,9 @@ abstract contract Burn_Integration_Concrete_Test is Integration_Test, Lockup_Int
         givenStreamHasNotBeenDepleted
     {
         vm.warp({ timestamp: defaults.CLIFF_TIME() });
+        changePrank({ msgSender: users.sender });
         lockup.cancel(streamId);
+        changePrank({ msgSender: users.recipient });
         vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2Lockup_StreamNotDepleted.selector, streamId));
         lockup.burn(streamId);
     }