From bdd8015f2c715ca6f73f46a18a21e81d67fc1e64 Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Fri, 19 Jul 2024 10:45:54 +0300 Subject: [PATCH 1/6] fix(invoice-module): check for invoice recipient/stream sender on 'cancelInvoice' exec --- src/modules/invoice-module/InvoiceModule.sol | 24 ++++++++++++------- .../interfaces/IInvoiceModule.sol | 3 +++ .../invoice-module/libraries/Errors.sol | 2 +- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/modules/invoice-module/InvoiceModule.sol b/src/modules/invoice-module/InvoiceModule.sol index 0cef083..6b8af2a 100644 --- a/src/modules/invoice-module/InvoiceModule.sol +++ b/src/modules/invoice-module/InvoiceModule.sol @@ -213,27 +213,33 @@ contract InvoiceModule is IInvoiceModule, StreamManager { if (invoice.status == Types.Status.Paid) { revert Errors.CannotCancelPaidInvoice(); } else if (invoice.status == Types.Status.Canceled) { - revert Errors.CannotCancelCanceledInvoice(); + revert Errors.InvoiceAlreadyCanceled(); } // Checks: the `msg.sender` is the creator if dealing with a transfer-based invoice + // or a linear/tranched stream-based invoice which was not paid yet (not streaming) // // Notes: - // - for a linear or tranched stream-based invoice, the `msg.sender` is checked in the + // - Once a linear or tranched stream is created, the `msg.sender` is checked in the // {SablierV2Lockup} `cancel` method - if (invoice.payment.method == Types.Method.Transfer) { + if (invoice.payment.method == Types.Method.Transfer || invoice.status == Types.Status.Pending) { if (invoice.recipient != msg.sender) { revert Errors.InvoiceOwnerUnauthorized(); } } - // Effects: cancel the stream accordingly depending on its type - if (invoice.payment.method == Types.Method.LinearStream) { - cancelLinearStream({ streamId: invoice.payment.streamId }); - } else if (invoice.payment.method == Types.Method.TranchedStream) { - cancelTranchedStream({ streamId: invoice.payment.streamId }); + // + // Notes: + // - A transfer-based invoice can be canceled directly + // - A linear or tranched stream MUST be canceled by calling the `cancel` method on the according + // {ISablierV2Lockup} contract + else if (invoice.status == Types.Status.Ongoing) { + if (invoice.payment.method == Types.Method.LinearStream) { + cancelLinearStream({ streamId: invoice.payment.streamId }); + } else if (invoice.payment.method == Types.Method.TranchedStream) { + cancelTranchedStream({ streamId: invoice.payment.streamId }); + } } - // Effects: mark the invoice as canceled _invoices[id].status = Types.Status.Canceled; diff --git a/src/modules/invoice-module/interfaces/IInvoiceModule.sol b/src/modules/invoice-module/interfaces/IInvoiceModule.sol index 00e6c18..ebb9a42 100644 --- a/src/modules/invoice-module/interfaces/IInvoiceModule.sol +++ b/src/modules/invoice-module/interfaces/IInvoiceModule.sol @@ -72,6 +72,9 @@ interface IInvoiceModule { /// @notice Cancels the `id` invoice /// /// Notes: + /// - A transfer-based invoice can be canceled only by its creator (recipient) + /// - A linear/tranched stream-based invoice can be canceled by its creator only if its + /// status is `Pending`; otherwise only the stream sender can cancel it /// - if the invoice has a linear or tranched stream payment method, the streaming flow will be /// stopped and the remaining funds will be refunded to the stream payer /// diff --git a/src/modules/invoice-module/libraries/Errors.sol b/src/modules/invoice-module/libraries/Errors.sol index 2275bf7..a2ec435 100644 --- a/src/modules/invoice-module/libraries/Errors.sol +++ b/src/modules/invoice-module/libraries/Errors.sol @@ -55,7 +55,7 @@ library Errors { error CannotCancelPaidInvoice(); /// @notice Thrown when an attempt is made to cancel an already canceled invoice - error CannotCancelCanceledInvoice(); + error InvoiceAlreadyCanceled(); /*////////////////////////////////////////////////////////////////////////// STREAM-MANAGER From b2594c5080e361fa7bbc4b424f2a23b4bb22c7b0 Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Fri, 19 Jul 2024 10:46:28 +0300 Subject: [PATCH 2/6] test(cancel-invoice): add basic integration 'cancelInvoice' tests --- .../cancel-invoice/cancelInvoice.t.sol | 170 ++++++++++++++++++ .../cancel-invoice/cancelInvoice.tree | 40 +++++ test/integration/shared/cancelInvoice.t.sol | 35 ++++ test/utils/Errors.sol | 9 + 4 files changed, 254 insertions(+) create mode 100644 test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.t.sol create mode 100644 test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.tree create mode 100644 test/integration/shared/cancelInvoice.t.sol diff --git a/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.t.sol b/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.t.sol new file mode 100644 index 0000000..dc43844 --- /dev/null +++ b/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.t.sol @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.26; + +import { CancelInvoice_Integration_Shared_Test } from "../../../shared/cancelInvoice.t.sol"; +import { Types } from "./../../../../../src/modules/invoice-module/libraries/Types.sol"; +import { Events } from "../../../../utils/Events.sol"; +import { Errors } from "../../../../utils/Errors.sol"; + +contract CancelInvoice_Integration_Concret_Test is CancelInvoice_Integration_Shared_Test { + function setUp() public virtual override { + CancelInvoice_Integration_Shared_Test.setUp(); + } + + function test_RevertWhen_InvoiceIsPaid() external { + // Set the one-off ETH transfer invoice as current one + uint256 invoiceId = 1; + + // Make Bob the payer for the default invoice + vm.startPrank({ msgSender: users.bob }); + + // Pay the invoice first + invoiceModule.payInvoice{ value: invoices[invoiceId].payment.amount }({ id: invoiceId }); + + // Make Eve the caller who is the recipient of the invoice + vm.startPrank({ msgSender: users.eve }); + + // Expect the call to revert with the {CannotCancelPaidInvoice} error + vm.expectRevert(Errors.CannotCancelPaidInvoice.selector); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + } + + function test_RevertWhen_InvoiceIsCanceled() external whenInvoiceNotAlreadyPaid { + // Set the one-off ETH transfer invoice as current one + uint256 invoiceId = 1; + + // Make Eve the caller who is the recipient of the invoice + vm.startPrank({ msgSender: users.eve }); + + // Cancel the invoice first + invoiceModule.cancelInvoice({ id: invoiceId }); + + // Expect the call to revert with the {InvoiceAlreadyCanceled} error + vm.expectRevert(Errors.InvoiceAlreadyCanceled.selector); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + } + + function test_RevertWhen_PaymentMethodTransfer_SenderNotInvoiceRecipient() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodTransfer + { + // Set the one-off ETH transfer invoice as current one + uint256 invoiceId = 1; + + // Make Bob the caller who IS NOT the recipient of the invoice + vm.startPrank({ msgSender: users.bob }); + + // Expect the call to revert with the {InvoiceOwnerUnauthorized} error + vm.expectRevert(Errors.InvoiceOwnerUnauthorized.selector); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + } + + function test_CancelInvoice_PaymentMethodTransfer() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodTransfer + whenSenderInvoiceRecipient + { + // Set the one-off ETH transfer invoice as current one + uint256 invoiceId = 1; + + // Make Eve the caller who is the recipient of the invoice + vm.startPrank({ msgSender: users.eve }); + + // Expect the {InvoiceCanceled} event to be emitted + vm.expectEmit(); + emit Events.InvoiceCanceled({ id: invoiceId }); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + + // Assert the actual and expected invoice status + Types.Invoice memory invoice = invoiceModule.getInvoice({ id: invoiceId }); + assertEq(uint8(invoice.status), uint8(Types.Status.Canceled)); + } + + function test_RevertWhen_PaymentMethodLinearStream_StatusPending_SenderNotInvoiceRecipient() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodLinearStream + givenInvoiceStatusPending + { + // Set current invoice as a linear stream-based one + uint256 invoiceId = 3; + + // Make Bob the caller who IS NOT the recipient of the invoice + vm.startPrank({ msgSender: users.bob }); + + // Expect the call to revert with the {InvoiceOwnerUnauthorized} error + vm.expectRevert(Errors.InvoiceOwnerUnauthorized.selector); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + } + + function test_CancelInvoice_PaymentMethodLinearStream_StatusPending() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodLinearStream + givenInvoiceStatusPending + whenSenderInvoiceRecipient + { + // Set current invoice as a linear stream-based one + uint256 invoiceId = 3; + + // Make Eve the caller who is the recipient of the invoice + vm.startPrank({ msgSender: users.eve }); + + // Expect the {InvoiceCanceled} event to be emitted + vm.expectEmit(); + emit Events.InvoiceCanceled({ id: invoiceId }); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + + // Assert the actual and expected invoice status + Types.Invoice memory invoice = invoiceModule.getInvoice({ id: invoiceId }); + assertEq(uint8(invoice.status), uint8(Types.Status.Canceled)); + } + + function test_RevertWhen_PaymentMethodLinearStream_StatusOngoing_SenderNotStreamSender() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodLinearStream + givenInvoiceStatusOngoing + { + // Set current invoice as a linear stream-based one + uint256 invoiceId = 3; + + // The invoice must be paid for its status to be updated to `Ongoing` + // Make Bob the payer of the invoice (also Bob will be the stream sender) + vm.startPrank({ msgSender: users.bob }); + + // Approve the {InvoiceModule} to transfer the USDT tokens on Bob's behalf + usdt.approve({ spender: address(invoiceModule), amount: invoices[invoiceId].payment.amount }); + + // Pay the invoice first (status will be updated to `Ongoing`) + invoiceModule.payInvoice{ value: invoices[invoiceId].payment.amount }({ id: invoiceId }); + + // Make Eve the caller who IS NOT the stream sender + vm.startPrank({ msgSender: users.eve }); + + // Expect the call to revert with the {SablierV2Lockup_Unauthorized} error + vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2Lockup_Unauthorized.selector, 1, users.bob)); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + } +} diff --git a/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.tree b/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.tree new file mode 100644 index 0000000..374a0b0 --- /dev/null +++ b/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.tree @@ -0,0 +1,40 @@ +cancelInvoice.t.sol +├── when the invoice status IS Paid +│ └── it should revert with the {CannotCancelPaidInvoice} error +└── when the invoice status IS NOT Paid + ├── when the invoice status IS Canceled + │ └── it should revert with the {InvoiceAlreadyCanceled} error + └── when the invoice status IS NOT Canceled + ├── given the payment method is transfer + │ ├── when the sender IS NOT the invoice recipient + │ │ └── it should revert with the {InvoiceOwnerUnauthorized} + │ └── when the sender IS the invoice recipient + │ ├── it should mark the invoice as Canceled + │ └── it should emit an {InvoiceCanceled} event + ├── given the payment method is linear stream-based + │ ├── given the invoice status is Pending + │ │ ├── when the sender IS NOT the invoice recipient + │ │ │ └── it should revert with the {InvoiceOwnerUnauthorized} + │ │ └── when the sender IS the invoice recipient + │ │ ├── it should mark the invoice as Canceled + │ │ └── it should emit an {InvoiceCanceled} event + │ └── given the invoice status is Ongoing + │ ├── when the sender IS NOT the stream's sender + │ │ └── it should revert with the {SablierV2Lockup_Unauthorized} error + │ └── when the sender IS the stream's sender + │ ├── it should mark the invoice as Canceled + │ └── it should emit an {InvoiceCanceled} event + └── given the payment method is tranched stream-based + ├── given the invoice status is Pending + │ ├── when the sender IS NOT the invoice recipient + │ │ └── it should revert with the {InvoiceOwnerUnauthorized} + │ └── when the sender IS the invoice recipient + │ ├── it should mark the invoice as Canceled + │ └── it should emit an {InvoiceCanceled} event + └── given the invoice status is Ongoing + ├── when the sender IS NOT the stream's sender + │ └──it should revert with the {SablierV2Lockup_Unauthorized} error + └── when the sender IS the stream's sender + ├── it should mark the invoice as Canceled + └── it should emit an {InvoiceCanceled} event + diff --git a/test/integration/shared/cancelInvoice.t.sol b/test/integration/shared/cancelInvoice.t.sol new file mode 100644 index 0000000..b707fae --- /dev/null +++ b/test/integration/shared/cancelInvoice.t.sol @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.26; + +import { Integration_Test } from "../Integration.t.sol"; +import { PayInvoice_Integration_Shared_Test } from "./payInvoice.t.sol"; + +abstract contract CancelInvoice_Integration_Shared_Test is Integration_Test, PayInvoice_Integration_Shared_Test { + function setUp() public virtual override(Integration_Test, PayInvoice_Integration_Shared_Test) { + PayInvoice_Integration_Shared_Test.setUp(); + } + + modifier whenInvoiceStatusNotPaid() { + _; + } + + modifier whenInvoiceStatusNotCanceled() { + _; + } + + modifier whenSenderInvoiceRecipient() { + _; + } + + modifier givenInvoiceStatusPending() { + _; + } + + modifier givenInvoiceStatusOngoing() { + _; + } + + modifier whenSenderStreamSender() { + _; + } +} diff --git a/test/utils/Errors.sol b/test/utils/Errors.sol index 13ce956..2ef1b86 100644 --- a/test/utils/Errors.sol +++ b/test/utils/Errors.sol @@ -83,10 +83,19 @@ library Errors { /// @notice Thrown when a tranched stream has a one-off recurrence type error TranchedStreamInvalidOneOffRecurence(); + /// @notice Thrown when an attempt is made to cancel an already paid invoice + error CannotCancelPaidInvoice(); + + /// @notice Thrown when an attempt is made to cancel an already canceled invoice + error InvoiceAlreadyCanceled(); + /*////////////////////////////////////////////////////////////////////////// STREAM-MANAGER //////////////////////////////////////////////////////////////////////////*/ /// @notice Thrown when the caller is not the broker admin error OnlyBrokerAdmin(); + + /// @notice Thrown when `msg.sender` is not the stream's sender + error SablierV2Lockup_Unauthorized(uint256 streamId, address caller); } From 3dc9ae3fd016783368d3332c97e1804cb946ac08 Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Mon, 22 Jul 2024 11:47:51 +0300 Subject: [PATCH 3/6] feat(stream-manager): make 'InvoiceModule' the stream sender and add 'onlyInitialStreamSender' modifier to guard management actions --- .../invoice-module/libraries/Errors.sol | 3 ++ .../sablier-v2/StreamManager.sol | 33 ++++++++++++++++--- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/modules/invoice-module/libraries/Errors.sol b/src/modules/invoice-module/libraries/Errors.sol index a2ec435..b5d1523 100644 --- a/src/modules/invoice-module/libraries/Errors.sol +++ b/src/modules/invoice-module/libraries/Errors.sol @@ -57,6 +57,9 @@ library Errors { /// @notice Thrown when an attempt is made to cancel an already canceled invoice error InvoiceAlreadyCanceled(); + /// @notice Thrown when the caller is not the initial stream sender + error OnlyInitialStreamSender(address initialSender); + /*////////////////////////////////////////////////////////////////////////// STREAM-MANAGER //////////////////////////////////////////////////////////////////////////*/ diff --git a/src/modules/invoice-module/sablier-v2/StreamManager.sol b/src/modules/invoice-module/sablier-v2/StreamManager.sol index 7dc908b..a890638 100644 --- a/src/modules/invoice-module/sablier-v2/StreamManager.sol +++ b/src/modules/invoice-module/sablier-v2/StreamManager.sol @@ -35,6 +35,13 @@ contract StreamManager is IStreamManager { /// @inheritdoc IStreamManager UD60x18 public override brokerFee; + /*////////////////////////////////////////////////////////////////////////// + PRIVATE STORAGE + //////////////////////////////////////////////////////////////////////////*/ + + /// @dev Stores the initial address of the account that started the stream + mapping(uint256 streamId => address initialSender) private _initialStreamSender; + /*////////////////////////////////////////////////////////////////////////// CONSTRUCTOR //////////////////////////////////////////////////////////////////////////*/ @@ -61,6 +68,13 @@ contract StreamManager is IStreamManager { _; } + /// @notice Reverts if the `msg.sender` is not the initial stream sender (creator of the stream) + modifier onlyInitialStreamSender(uint256 streamId) { + address initialSender = _initialStreamSender[streamId]; + if (msg.sender != initialSender) revert Errors.OnlyInitialStreamSender(initialSender); + _; + } + /*////////////////////////////////////////////////////////////////////////// NON-CONSTANT FUNCTIONS //////////////////////////////////////////////////////////////////////////*/ @@ -78,6 +92,9 @@ contract StreamManager is IStreamManager { // Create the Lockup Linear stream streamId = _createLinearStream(asset, totalAmount, startTime, endTime, recipient); + + // Set `msg.sender` as the initial stream sender to allow secure stream management + _initialStreamSender[streamId] = msg.sender; } /// @inheritdoc IStreamManager @@ -94,6 +111,9 @@ contract StreamManager is IStreamManager { // Create the Lockup Linear stream streamId = _createTranchedStream(asset, totalAmount, startTime, recipient, numberOfTranches, recurrence); + + // Set `msg.sender` as the initial stream sender to allow secure stream management + _initialStreamSender[streamId] = msg.sender; } /// @inheritdoc IStreamManager @@ -165,7 +185,7 @@ contract StreamManager is IStreamManager { LockupLinear.CreateWithTimestamps memory params; // Declare the function parameters - params.sender = msg.sender; // The sender will be able to cancel the stream + params.sender = address(this); // The sender will be able to cancel the stream params.recipient = recipient; // The recipient of the streamed assets params.totalAmount = totalAmount; // Total amount is the amount inclusive of all fees params.asset = asset; // The streaming asset @@ -193,7 +213,7 @@ contract StreamManager is IStreamManager { LockupTranched.CreateWithTimestamps memory params; // Declare the function parameters - params.sender = msg.sender; // The sender will be able to cancel the stream + params.sender = address(this); // The sender will be able to cancel the stream params.recipient = recipient; // The recipient of the streamed assets params.totalAmount = totalAmount; // Total amount is the amount inclusive of all fees params.asset = asset; // The streaming asset @@ -227,12 +247,17 @@ contract StreamManager is IStreamManager { } /// @dev Withdraws from either a linear or tranched stream - function _withdrawStream(ISablierV2Lockup sablier, uint256 streamId, address to, uint128 amount) internal { + function _withdrawStream( + ISablierV2Lockup sablier, + uint256 streamId, + address to, + uint128 amount + ) internal onlyInitialStreamSender(streamId) { sablier.withdraw(streamId, to, amount); } /// @dev Cancels the `streamId` stream - function _cancelStream(ISablierV2Lockup sablier, uint256 streamId) internal { + function _cancelStream(ISablierV2Lockup sablier, uint256 streamId) internal onlyInitialStreamSender(streamId) { sablier.cancel(streamId); } From 270b5ddcabdfeb236bc5f2ae6dd1be623bcb9c14 Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Mon, 22 Jul 2024 11:48:45 +0300 Subject: [PATCH 4/6] test: fix 'payInvoice' due to new sender workflow and add missing error --- .../concrete/invoice-module/pay-invoice/payInvoice.t.sol | 4 ++-- test/utils/Errors.sol | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/test/integration/concrete/invoice-module/pay-invoice/payInvoice.t.sol b/test/integration/concrete/invoice-module/pay-invoice/payInvoice.t.sol index 996b248..45ca0e6 100644 --- a/test/integration/concrete/invoice-module/pay-invoice/payInvoice.t.sol +++ b/test/integration/concrete/invoice-module/pay-invoice/payInvoice.t.sol @@ -264,7 +264,7 @@ contract PayInvoice_Integration_Concret_Test is PayInvoice_Integration_Shared_Te // Assert the actual and the expected state of the Sablier v2 linear stream LockupLinear.StreamLL memory stream = invoiceModule.getLinearStream({ streamId: 1 }); - assertEq(stream.sender, users.bob); + assertEq(stream.sender, address(invoiceModule)); assertEq(stream.recipient, users.eve); assertEq(address(stream.asset), address(usdt)); assertEq(stream.startTime, invoice.startTime); @@ -316,7 +316,7 @@ contract PayInvoice_Integration_Concret_Test is PayInvoice_Integration_Shared_Te // Assert the actual and the expected state of the Sablier v2 tranched stream LockupTranched.StreamLT memory stream = invoiceModule.getTranchedStream({ streamId: 1 }); - assertEq(stream.sender, users.bob); + assertEq(stream.sender, address(invoiceModule)); assertEq(stream.recipient, users.eve); assertEq(address(stream.asset), address(usdt)); assertEq(stream.startTime, invoice.startTime); diff --git a/test/utils/Errors.sol b/test/utils/Errors.sol index 2ef1b86..697f76d 100644 --- a/test/utils/Errors.sol +++ b/test/utils/Errors.sol @@ -89,6 +89,9 @@ library Errors { /// @notice Thrown when an attempt is made to cancel an already canceled invoice error InvoiceAlreadyCanceled(); + /// @notice Thrown when the caller is not the initial stream sender + error OnlyInitialStreamSender(address initialSender); + /*////////////////////////////////////////////////////////////////////////// STREAM-MANAGER //////////////////////////////////////////////////////////////////////////*/ From e817bee0a08e26c6672c5872b242d8397c0ea1c8 Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Mon, 22 Jul 2024 11:48:59 +0300 Subject: [PATCH 5/6] test: add missing 'cancelInvoice' integration tests --- .../cancel-invoice/cancelInvoice.t.sol | 150 +++++++++++++++++- .../cancel-invoice/cancelInvoice.tree | 12 +- test/integration/shared/cancelInvoice.t.sol | 2 +- 3 files changed, 153 insertions(+), 11 deletions(-) diff --git a/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.t.sol b/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.t.sol index dc43844..77487ee 100644 --- a/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.t.sol +++ b/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.t.sol @@ -138,7 +138,7 @@ contract CancelInvoice_Integration_Concret_Test is CancelInvoice_Integration_Sha assertEq(uint8(invoice.status), uint8(Types.Status.Canceled)); } - function test_RevertWhen_PaymentMethodLinearStream_StatusOngoing_SenderNotStreamSender() + function test_RevertWhen_PaymentMethodLinearStream_StatusOngoing_SenderNoInitialtStreamSender() external whenInvoiceNotAlreadyPaid whenInvoiceNotCanceled @@ -158,13 +158,155 @@ contract CancelInvoice_Integration_Concret_Test is CancelInvoice_Integration_Sha // Pay the invoice first (status will be updated to `Ongoing`) invoiceModule.payInvoice{ value: invoices[invoiceId].payment.amount }({ id: invoiceId }); - // Make Eve the caller who IS NOT the stream sender + // Make Eve the caller who IS NOT the initial stream sender but rather the recipient vm.startPrank({ msgSender: users.eve }); - // Expect the call to revert with the {SablierV2Lockup_Unauthorized} error - vm.expectRevert(abi.encodeWithSelector(Errors.SablierV2Lockup_Unauthorized.selector, 1, users.bob)); + // Expect the call to revert with the {OnlyInitialStreamSender} error + vm.expectRevert(abi.encodeWithSelector(Errors.OnlyInitialStreamSender.selector, users.bob)); // Run the test invoiceModule.cancelInvoice({ id: invoiceId }); } + + function test_CancelInvoice_PaymentMethodLinearStream_StatusOngoing() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodLinearStream + givenInvoiceStatusOngoing + whenSenderInitialStreamSender + { + // Set current invoice as a linear stream-based one + uint256 invoiceId = 3; + + // The invoice must be paid for its status to be updated to `Ongoing` + // Make Bob the payer of the invoice (also Bob will be the initial stream sender) + vm.startPrank({ msgSender: users.bob }); + + // Approve the {InvoiceModule} to transfer the USDT tokens on Bob's behalf + usdt.approve({ spender: address(invoiceModule), amount: invoices[invoiceId].payment.amount }); + + // Pay the invoice first (status will be updated to `Ongoing`) + invoiceModule.payInvoice{ value: invoices[invoiceId].payment.amount }({ id: invoiceId }); + + // Expect the {InvoiceCanceled} event to be emitted + vm.expectEmit(); + emit Events.InvoiceCanceled({ id: invoiceId }); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + + // Assert the actual and expected invoice status + Types.Invoice memory invoice = invoiceModule.getInvoice({ id: invoiceId }); + assertEq(uint8(invoice.status), uint8(Types.Status.Canceled)); + } + + function test_RevertWhen_PaymentMethodTranchedStream_StatusPending_SenderNotInvoiceRecipient() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodTranchedStream + givenInvoiceStatusPending + { + // Set current invoice as a tranched stream-based one + uint256 invoiceId = 4; + + // Make Bob the caller who IS NOT the recipient of the invoice + vm.startPrank({ msgSender: users.bob }); + + // Expect the call to revert with the {InvoiceOwnerUnauthorized} error + vm.expectRevert(Errors.InvoiceOwnerUnauthorized.selector); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + } + + function test_CancelInvoice_PaymentMethodTranchedStream_StatusPending() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodTranchedStream + givenInvoiceStatusPending + whenSenderInvoiceRecipient + { + // Set current invoice as a tranched stream-based one + uint256 invoiceId = 4; + + // Make Eve the caller who is the recipient of the invoice + vm.startPrank({ msgSender: users.eve }); + + // Expect the {InvoiceCanceled} event to be emitted + vm.expectEmit(); + emit Events.InvoiceCanceled({ id: invoiceId }); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + + // Assert the actual and expected invoice status + Types.Invoice memory invoice = invoiceModule.getInvoice({ id: invoiceId }); + assertEq(uint8(invoice.status), uint8(Types.Status.Canceled)); + } + + function test_RevertWhen_PaymentMethodTranchedStream_StatusOngoing_SenderNoInitialtStreamSender() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodTranchedStream + givenInvoiceStatusOngoing + { + // Set current invoice as a tranched stream-based one + uint256 invoiceId = 4; + + // The invoice must be paid for its status to be updated to `Ongoing` + // Make Bob the payer of the invoice (also Bob will be the stream sender) + vm.startPrank({ msgSender: users.bob }); + + // Approve the {InvoiceModule} to transfer the USDT tokens on Bob's behalf + usdt.approve({ spender: address(invoiceModule), amount: invoices[invoiceId].payment.amount }); + + // Pay the invoice first (status will be updated to `Ongoing`) + invoiceModule.payInvoice{ value: invoices[invoiceId].payment.amount }({ id: invoiceId }); + + // Make Eve the caller who IS NOT the initial stream sender but rather the recipient + vm.startPrank({ msgSender: users.eve }); + + // Expect the call to revert with the {OnlyInitialStreamSender} error + vm.expectRevert(abi.encodeWithSelector(Errors.OnlyInitialStreamSender.selector, users.bob)); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + } + + function test_CancelInvoice_PaymentMethodTranchedStream_StatusOngoing() + external + whenInvoiceNotAlreadyPaid + whenInvoiceNotCanceled + givenPaymentMethodTranchedStream + givenInvoiceStatusOngoing + whenSenderInitialStreamSender + { + // Set current invoice as a tranched stream-based one + uint256 invoiceId = 4; + + // The invoice must be paid for its status to be updated to `Ongoing` + // Make Bob the payer of the invoice (also Bob will be the initial stream sender) + vm.startPrank({ msgSender: users.bob }); + + // Approve the {InvoiceModule} to transfer the USDT tokens on Bob's behalf + usdt.approve({ spender: address(invoiceModule), amount: invoices[invoiceId].payment.amount }); + + // Pay the invoice first (status will be updated to `Ongoing`) + invoiceModule.payInvoice{ value: invoices[invoiceId].payment.amount }({ id: invoiceId }); + + // Expect the {InvoiceCanceled} event to be emitted + vm.expectEmit(); + emit Events.InvoiceCanceled({ id: invoiceId }); + + // Run the test + invoiceModule.cancelInvoice({ id: invoiceId }); + + // Assert the actual and expected invoice status + Types.Invoice memory invoice = invoiceModule.getInvoice({ id: invoiceId }); + assertEq(uint8(invoice.status), uint8(Types.Status.Canceled)); + } } diff --git a/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.tree b/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.tree index 374a0b0..58202e3 100644 --- a/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.tree +++ b/test/integration/concrete/invoice-module/cancel-invoice/cancelInvoice.tree @@ -19,9 +19,9 @@ cancelInvoice.t.sol │ │ ├── it should mark the invoice as Canceled │ │ └── it should emit an {InvoiceCanceled} event │ └── given the invoice status is Ongoing - │ ├── when the sender IS NOT the stream's sender - │ │ └── it should revert with the {SablierV2Lockup_Unauthorized} error - │ └── when the sender IS the stream's sender + │ ├── when the sender IS NOT the initial stream sender + │ │ └── it should revert with the {OnlyInitialStreamSender} error + │ └── when the sender IS the initial stream sender │ ├── it should mark the invoice as Canceled │ └── it should emit an {InvoiceCanceled} event └── given the payment method is tranched stream-based @@ -32,9 +32,9 @@ cancelInvoice.t.sol │ ├── it should mark the invoice as Canceled │ └── it should emit an {InvoiceCanceled} event └── given the invoice status is Ongoing - ├── when the sender IS NOT the stream's sender - │ └──it should revert with the {SablierV2Lockup_Unauthorized} error - └── when the sender IS the stream's sender + ├── when the sender IS NOT the initial stream sender + │ └──it should revert with the {OnlyInitialStreamSender} error + └── when the sender IS the initial stream sender ├── it should mark the invoice as Canceled └── it should emit an {InvoiceCanceled} event diff --git a/test/integration/shared/cancelInvoice.t.sol b/test/integration/shared/cancelInvoice.t.sol index b707fae..e8cfcf2 100644 --- a/test/integration/shared/cancelInvoice.t.sol +++ b/test/integration/shared/cancelInvoice.t.sol @@ -29,7 +29,7 @@ abstract contract CancelInvoice_Integration_Shared_Test is Integration_Test, Pay _; } - modifier whenSenderStreamSender() { + modifier whenSenderInitialStreamSender() { _; } } From 6879df900a7a4dfeb790a46bb6698d6c454320ef Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Mon, 22 Jul 2024 12:36:37 +0300 Subject: [PATCH 6/6] fix(stream-manager): replace 'onlyInitialStreamSender' modifier with inline check, make 'StreamManager' abstract and improve docs --- .../sablier-v2/StreamManager.sol | 31 +++++++++---------- .../sablier-v2/interfaces/IStreamManager.sol | 6 ++++ 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/modules/invoice-module/sablier-v2/StreamManager.sol b/src/modules/invoice-module/sablier-v2/StreamManager.sol index a890638..258a1a9 100644 --- a/src/modules/invoice-module/sablier-v2/StreamManager.sol +++ b/src/modules/invoice-module/sablier-v2/StreamManager.sol @@ -16,7 +16,7 @@ import { Types } from "./../libraries/Types.sol"; /// @title StreamManager /// @dev See the documentation in {IStreamManager} -contract StreamManager is IStreamManager { +abstract contract StreamManager is IStreamManager { using SafeERC20 for IERC20; /*////////////////////////////////////////////////////////////////////////// @@ -40,6 +40,8 @@ contract StreamManager is IStreamManager { //////////////////////////////////////////////////////////////////////////*/ /// @dev Stores the initial address of the account that started the stream + /// By default, each stream will be created by this contract (the sender address of each stream will be address(this)) + /// therefore this mapping is used to allow only authorized senders to execute management-related actions i.e. cancellations mapping(uint256 streamId => address initialSender) private _initialStreamSender; /*////////////////////////////////////////////////////////////////////////// @@ -68,13 +70,6 @@ contract StreamManager is IStreamManager { _; } - /// @notice Reverts if the `msg.sender` is not the initial stream sender (creator of the stream) - modifier onlyInitialStreamSender(uint256 streamId) { - address initialSender = _initialStreamSender[streamId]; - if (msg.sender != initialSender) revert Errors.OnlyInitialStreamSender(initialSender); - _; - } - /*////////////////////////////////////////////////////////////////////////// NON-CONSTANT FUNCTIONS //////////////////////////////////////////////////////////////////////////*/ @@ -93,7 +88,7 @@ contract StreamManager is IStreamManager { // Create the Lockup Linear stream streamId = _createLinearStream(asset, totalAmount, startTime, endTime, recipient); - // Set `msg.sender` as the initial stream sender to allow secure stream management + // Set `msg.sender` as the initial stream sender to allow authenticated stream management _initialStreamSender[streamId] = msg.sender; } @@ -112,7 +107,7 @@ contract StreamManager is IStreamManager { // Create the Lockup Linear stream streamId = _createTranchedStream(asset, totalAmount, startTime, recipient, numberOfTranches, recurrence); - // Set `msg.sender` as the initial stream sender to allow secure stream management + // Set `msg.sender` as the initial stream sender to allow authenticated stream management _initialStreamSender[streamId] = msg.sender; } @@ -145,11 +140,13 @@ contract StreamManager is IStreamManager { /// @inheritdoc IStreamManager function cancelLinearStream(uint256 streamId) public { + // Checks, Effect, Interactions _cancelStream({ sablier: LOCKUP_LINEAR, streamId: streamId }); } /// @inheritdoc IStreamManager function cancelTranchedStream(uint256 streamId) public { + // Checks, Effect, Interactions _cancelStream({ sablier: LOCKUP_TRANCHED, streamId: streamId }); } @@ -247,17 +244,17 @@ contract StreamManager is IStreamManager { } /// @dev Withdraws from either a linear or tranched stream - function _withdrawStream( - ISablierV2Lockup sablier, - uint256 streamId, - address to, - uint128 amount - ) internal onlyInitialStreamSender(streamId) { + function _withdrawStream(ISablierV2Lockup sablier, uint256 streamId, address to, uint128 amount) internal { sablier.withdraw(streamId, to, amount); } /// @dev Cancels the `streamId` stream - function _cancelStream(ISablierV2Lockup sablier, uint256 streamId) internal onlyInitialStreamSender(streamId) { + function _cancelStream(ISablierV2Lockup sablier, uint256 streamId) internal { + // Checks: the `msg.sender` is the initial stream creator + address initialSender = _initialStreamSender[streamId]; + if (msg.sender != initialSender) revert Errors.OnlyInitialStreamSender(initialSender); + + // Cancel the stream sablier.cancel(streamId); } diff --git a/src/modules/invoice-module/sablier-v2/interfaces/IStreamManager.sol b/src/modules/invoice-module/sablier-v2/interfaces/IStreamManager.sol index 9202263..c39dece 100644 --- a/src/modules/invoice-module/sablier-v2/interfaces/IStreamManager.sol +++ b/src/modules/invoice-module/sablier-v2/interfaces/IStreamManager.sol @@ -100,8 +100,14 @@ interface IStreamManager { function withdrawTranchedStream(uint256 streamId, address to, uint128 amount) external; /// @notice See the documentation in {ISablierV2Lockup-cancel} + /// + /// Notes: + /// - Reverts with {OnlyInitialStreamSender} if `msg.sender` is not the initial stream creator function cancelLinearStream(uint256 streamId) external; /// @notice See the documentation in {ISablierV2Lockup-cancel} + /// + /// Notes: + /// - Reverts with {OnlyInitialStreamSender} if `msg.sender` is not the initial stream creator function cancelTranchedStream(uint256 streamId) external; }