From eefae0467706fcc551f9d7d601e3489b32fe3778 Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Thu, 18 Jul 2024 16:00:09 +0300 Subject: [PATCH 1/7] fix(invoice-module): linear/tranched stream-based invoice status on 'payInvoice' exec --- src/modules/invoice-module/InvoiceModule.sol | 6 ++++-- .../concrete/invoice-module/pay-invoice/payInvoice.t.sol | 8 ++++---- .../concrete/invoice-module/pay-invoice/payInvoice.tree | 2 ++ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/modules/invoice-module/InvoiceModule.sol b/src/modules/invoice-module/InvoiceModule.sol index 14a0f3c..0cef083 100644 --- a/src/modules/invoice-module/InvoiceModule.sol +++ b/src/modules/invoice-module/InvoiceModule.sol @@ -185,6 +185,7 @@ contract InvoiceModule is IInvoiceModule, StreamManager { // Handle the payment workflow depending on the payment method type if (invoice.payment.method == Types.Method.Transfer) { + // Effects: pay the invoice and update its status to `Paid` or `Ongoing` depending on the payment type _payByTransfer(id, invoice); } else { uint256 streamId; @@ -193,8 +194,9 @@ contract InvoiceModule is IInvoiceModule, StreamManager { streamId = _payByLinearStream(invoice); } else streamId = _payByTranchedStream(invoice); - // Effects: update the status of the invoice and stream ID - _invoices[id].status = Types.Status.Paid; + // Effects: update the status of the invoice to `Ongoing` and the stream ID + // if dealing with a linear or tranched-based invoice + _invoices[id].status = Types.Status.Ongoing; _invoices[id].payment.streamId = streamId; } 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 c672278..83b5f94 100644 --- a/test/integration/concrete/invoice-module/pay-invoice/payInvoice.t.sol +++ b/test/integration/concrete/invoice-module/pay-invoice/payInvoice.t.sol @@ -267,7 +267,7 @@ contract PayInvoice_Integration_Concret_Test is PayInvoice_Integration_Shared_Te emit Events.InvoicePaid({ id: invoiceId, payer: users.bob, - status: Types.Status.Paid, + status: Types.Status.Ongoing, payment: Types.Payment({ method: invoices[invoiceId].payment.method, recurrence: invoices[invoiceId].payment.recurrence, @@ -283,7 +283,7 @@ contract PayInvoice_Integration_Concret_Test is PayInvoice_Integration_Shared_Te // Assert the actual and the expected state of the invoice Types.Invoice memory invoice = invoiceModule.getInvoice({ id: invoiceId }); - assertEq(uint8(invoice.status), uint8(Types.Status.Paid)); + assertEq(uint8(invoice.status), uint8(Types.Status.Ongoing)); assertEq(invoice.payment.streamId, 1); assertEq(invoice.payment.paymentsLeft, 0); @@ -319,7 +319,7 @@ contract PayInvoice_Integration_Concret_Test is PayInvoice_Integration_Shared_Te emit Events.InvoicePaid({ id: invoiceId, payer: users.bob, - status: Types.Status.Paid, + status: Types.Status.Ongoing, payment: Types.Payment({ method: invoices[invoiceId].payment.method, recurrence: invoices[invoiceId].payment.recurrence, @@ -335,7 +335,7 @@ contract PayInvoice_Integration_Concret_Test is PayInvoice_Integration_Shared_Te // Assert the actual and the expected state of the invoice Types.Invoice memory invoice = invoiceModule.getInvoice({ id: invoiceId }); - assertEq(uint8(invoice.status), uint8(Types.Status.Paid)); + assertEq(uint8(invoice.status), uint8(Types.Status.Ongoing)); assertEq(invoice.payment.streamId, 1); assertEq(invoice.payment.paymentsLeft, 0); diff --git a/test/integration/concrete/invoice-module/pay-invoice/payInvoice.tree b/test/integration/concrete/invoice-module/pay-invoice/payInvoice.tree index c42e37e..084ff6d 100644 --- a/test/integration/concrete/invoice-module/pay-invoice/payInvoice.tree +++ b/test/integration/concrete/invoice-module/pay-invoice/payInvoice.tree @@ -29,10 +29,12 @@ payInvoice.t.sol │ └── it should emit an {InvoicePaid} event ├── given the payment method is linear stream │ ├── it should create a Sablier v2 linear stream + │ ├── it should update the invoice status to Ongoing │ ├── it should update the invoice stream ID │ └── it should emit an {InvoicePaid} event └── given the payment method is tranched stream ├── it should create a Sablier v2 tranched stream + ├── it should update the invoice status to Ongoing ├── it should update the invoice stream ID └── it should emit an {InvoicePaid} event From ced9ee71af8c255522bbeaa9090b5e9191017ecc Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Fri, 19 Jul 2024 10:43:40 +0300 Subject: [PATCH 2/7] test(pay-invoice): create mock invoices in the shared 'payInvoice.t.sol' file --- .../pay-invoice/payInvoice.t.sol | 25 ------------------- test/integration/shared/payInvoice.t.sol | 25 +++++++++++++++++++ 2 files changed, 25 insertions(+), 25 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 83b5f94..996b248 100644 --- a/test/integration/concrete/invoice-module/pay-invoice/payInvoice.t.sol +++ b/test/integration/concrete/invoice-module/pay-invoice/payInvoice.t.sol @@ -11,31 +11,6 @@ import { LockupLinear, LockupTranched } from "@sablier/v2-core/src/types/DataTyp contract PayInvoice_Integration_Concret_Test is PayInvoice_Integration_Shared_Test { function setUp() public virtual override { PayInvoice_Integration_Shared_Test.setUp(); - - // Create a mock invoice with a one-off USDT transfer - Types.Invoice memory invoice = createInvoiceWithOneOffTransfer({ asset: address(usdt), recipient: users.eve }); - invoices[0] = invoice; - executeCreateInvoice({ invoice: invoice, user: users.eve }); - - // Create a mock invoice with a one-off ETH transfer - invoice = createInvoiceWithOneOffTransfer({ asset: address(0), recipient: users.eve }); - invoices[1] = invoice; - executeCreateInvoice({ invoice: invoice, user: users.eve }); - - // Create a mock invoice with a recurring USDT transfer - invoice = createInvoiceWithRecurringTransfer({ recurrence: Types.Recurrence.Weekly, recipient: users.eve }); - invoices[2] = invoice; - executeCreateInvoice({ invoice: invoice, user: users.eve }); - - // Create a mock invoice with a linear stream payment - invoice = createInvoiceWithLinearStream({ recipient: users.eve }); - invoices[3] = invoice; - executeCreateInvoice({ invoice: invoice, user: users.eve }); - - // Create a mock invoice with a tranched stream payment - invoice = createInvoiceWithTranchedStream({ recurrence: Types.Recurrence.Weekly, recipient: users.eve }); - invoices[4] = invoice; - executeCreateInvoice({ invoice: invoice, user: users.eve }); } function test_RevertWhen_InvoiceNull() external { diff --git a/test/integration/shared/payInvoice.t.sol b/test/integration/shared/payInvoice.t.sol index bf01a99..0a4658c 100644 --- a/test/integration/shared/payInvoice.t.sol +++ b/test/integration/shared/payInvoice.t.sol @@ -10,6 +10,31 @@ abstract contract PayInvoice_Integration_Shared_Test is Integration_Test, Create function setUp() public virtual override(Integration_Test, CreateInvoice_Integration_Shared_Test) { CreateInvoice_Integration_Shared_Test.setUp(); + + // Create a mock invoice with a one-off USDT transfer + Types.Invoice memory invoice = createInvoiceWithOneOffTransfer({ asset: address(usdt), recipient: users.eve }); + invoices[0] = invoice; + executeCreateInvoice({ invoice: invoice, user: users.eve }); + + // Create a mock invoice with a one-off ETH transfer + invoice = createInvoiceWithOneOffTransfer({ asset: address(0), recipient: users.eve }); + invoices[1] = invoice; + executeCreateInvoice({ invoice: invoice, user: users.eve }); + + // Create a mock invoice with a recurring USDT transfer + invoice = createInvoiceWithRecurringTransfer({ recurrence: Types.Recurrence.Weekly, recipient: users.eve }); + invoices[2] = invoice; + executeCreateInvoice({ invoice: invoice, user: users.eve }); + + // Create a mock invoice with a linear stream payment + invoice = createInvoiceWithLinearStream({ recipient: users.eve }); + invoices[3] = invoice; + executeCreateInvoice({ invoice: invoice, user: users.eve }); + + // Create a mock invoice with a tranched stream payment + invoice = createInvoiceWithTranchedStream({ recurrence: Types.Recurrence.Weekly, recipient: users.eve }); + invoices[4] = invoice; + executeCreateInvoice({ invoice: invoice, user: users.eve }); } modifier whenInvoiceNotNull() { From bdd8015f2c715ca6f73f46a18a21e81d67fc1e64 Mon Sep 17 00:00:00 2001 From: gabrielstoica Date: Fri, 19 Jul 2024 10:45:54 +0300 Subject: [PATCH 3/7] 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 4/7] 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 5/7] 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 6/7] 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 7/7] 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() { _; } }