Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add protocol fee variable in claim and clawback functions #220

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Nov 20, 2023

The idea behind this is to add one more layer of security for users in case the protocol fee is changed after an airstream campaign is created.

This would have been an issue if the protocol fee is greater than 0 and there is no expiration, resulting in funds being locked and unable to be clawed back. Effectively, this would have allowed us to take a percentage of any airstream campaign that is created.

This PR fixes the following issue found by Zach Obront in his recent audit:

https://cantina.xyz/ai/6d6a47ff-5279-4502-8705-d837b159a31e/findings/e2ed29f3-34f2-4dc7-8849-38aa7f0c3b91

Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

See my review in the comments.

The only thing left to do is to update the clawback function to revert with a new custom error for the case when the protocol is non-zero.

src/abstracts/SablierV2MerkleStreamer.sol Outdated Show resolved Hide resolved
src/libraries/Errors.sol Outdated Show resolved Hide resolved
src/abstracts/SablierV2MerkleStreamer.sol Outdated Show resolved Hide resolved
src/abstracts/SablierV2MerkleStreamer.sol Outdated Show resolved Hide resolved
@PaulRBerg PaulRBerg force-pushed the feat/add-protocol-fee-variable branch from dadb407 to 78fdd52 Compare November 26, 2023 15:48
@andreivladbrg
Copy link
Member Author

andreivladbrg commented Nov 26, 2023

The only thing left to do is to update the clawback function to revert with a new custom error for the case when the protocol is non-zero.

To revert when the protocol fee is non-zero? you sure? i say that it shouldn't revert. there are 4 cases:

  • protocolFee == 0 && hasExpired == false --> revert
  • protocolFee == 0 && hasExpired == true --> can clawback
  • protocolFee > 0 && hasExpired == false --> can clawback
  • protocolFee > 0 && hasExpired == true --> can clawback

if the protocol fee is non-zero it means that claims are unavailable, and if the expiration is set to 0 how would someone claw back the funds if we are going to revert😅? am i missing smth?

@PaulRBerg
Copy link
Member

To revert when the protocol fee is non-zero? you sure?
if the protocol fee is non-zero it means that claims are unavailable, and if the expiration is set to 0 how would someone claw back the funds if we are going to revert😅?

LOL! Thanks for the correction.

am i missing smth?

Nah, I have missed something. Thanks.

andreivladbrg and others added 5 commits November 28, 2023 14:49
feat: allow clawbacks when protocol fee greater than zero and campaign not expired
test: update tests accordingly
docs: update natspec in ISablierV2MerkleStreamerFactory
style: remove unneeded new line
chore: fix order and formatting in "claim" tests
@andreivladbrg andreivladbrg force-pushed the feat/add-protocol-fee-variable branch from dce577a to 6eb2158 Compare November 28, 2023 12:59
@andreivladbrg
Copy link
Member Author

updated the precompiles, is this ready to be merged? @PaulRBerg

Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

lgtm now

@PaulRBerg PaulRBerg merged commit f1d6ec2 into staging Nov 28, 2023
7 checks passed
@PaulRBerg PaulRBerg deleted the feat/add-protocol-fee-variable branch November 28, 2023 14:34
andreivladbrg added a commit that referenced this pull request Dec 16, 2023
* feat: don't allow claims when protocol fee not zero

feat: allow clawbacks when protocol fee greater than zero and campaign not expired
test: update tests accordingly

* docs: specify the protocol fee requirement

docs: update natspec in ISablierV2MerkleStreamerFactory
style: remove unneeded new line

* refactor: change order in "claim"

chore: fix order and formatting in "claim" tests

* refactor: move protocol fee check at the bottom

* test: update Precompiles bytecode

---------

Co-authored-by: Paul Razvan Berg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants