sablier-labs/v2-periphery

Switch back to CREATE2 for creating the MerkleLockup campaigns

Closed this issue ยท 21 comments

Problem

The use of CREATE can be exploited when the blockchain is reorged. For more details, see M-04. Use of CREATE method is suspicious of reorg attack in CodeHawks' audit report.

My Thoughts

The odds for this vulnerability to occur are very low because stealing of funds can occur only in an exquisite set of circumstances:

  • Alice creates airstream campaign
  • Alice sends another tx to fund the campaign
    • Note: contrary to what Rusty said here, this issue applies even when Alice uses the same wallet for the funding tx because the higher-nonce tx may still be kept in the mempool
  • Alice's funding tx remains in the mempool WITHOUT being included in the previously reorged blockchain
  • Eve notices this setup and sends a tx to create an airstream campaign, sniping Alice's campaign contract
  • Alice's funding tx gets processed now and Eve clawbacks

Solution

  • Switch to CREATE2
  • Remove percentage sum check in create function and move it to the claim function
  • Add appropriate tests

Related

Rusty's finding during the private audit: https://github.com/cantinasec/review-sablier-3/issues/1

Instead of generating salt using just input parameters, how about we also add the followings to create2 salt:

  1. block.timestamp: all input parameters being same, block.timestamp is the only value that guarantees to be cahnged over time.

  2. block.chainid: Prevents cross-chain replay attacks. Re L-05 in case the same Airstream is deployed on multiple networks.

Doesn't add much gas, but lowers the chances of address collision.

Also, as a solution to this issue related to createMerkleLT, how about the following:

  1. Exclude tranchesWithPercentages from salt. So there is no need to revert in createMerkleLT execution.
  2. Store the total percentage sum as a IMMUTABLE in SablierV2MerkleLT contract.
  3. claim reverts if the constant value is not 100%.

Because its a constant, it would not add much gas overhead and at the same time, it prevents funds being stuck (counterfactual deployment) in case the percentages dont add up.

Tagging @razgraf because my last comment can impact UI as well.

Thanks for the tag @smol-ninja. We don't precompute the address in the UI as we try to enforce a synchronized flow (user first deploys the campaign, we get the address from an event log after which we allow them to fund it). If I didn't miss anything, feel free to approach this without having to worry about the UI.

My question is if a user deploys Airstream campaign for Lockup Tranched in which the percentages don't add up, can UI show it and display warning about it?

In the previous implementation, if the percentages dont add up to 100, user couldn't deploy the campaign contract. But Rusty raised a concern about this. Thus I am suggesting to allow deploying the campaign even if the percentages don't add up and put a check in claim. Re: this comment.

Interesting ideas @smol-ninja.

  • I am against including block.timestamp in the salt because it would be a footgun. Users might run an off-chain script for precomputing the address, thinking that they got it right, but then when the tx is included on the blockchain, the address would definitely be different (because the time of inclusion and hence block.timestamp are non-deterministic).

  • I am in favor of including block.chainid in the CREATE2 salt, though I don't think this will address L-05. In fact, I'm not even sure I understand why L-05 is an issue. I have asked Cyfrin for additional clarifications.


@razgraf Shub's change entails an additional check in the claim flow. If we accept his proposal, the claim function might revert if the tranche percentages don't add up to 100%.

I'm still thinking about whether doing that would be worth it. There would also be consequences for clawback.

  1. Valid point.
  2. I also now think that adding block.chainid offers no benefit at all. I was thinking about replay attack in the context of deploying the airstream campaign (i.e. using the same parameters, one can deploy the exact same contract on other chains but this is not an issue at all).

There would also be consequences for clawback

How does it affect clawback, @PaulRBerg ?

Clawback has to be refactored to include a new check for the tranche percentages. If they are not 100%, the admin must be able to always recover their funds, regardless of the other rules.

Clawback has to be refactored to include a new check for the tranche percentages

Admin can clawback until the first claim is made. So if the tranche percentages don't add up to 100, then admin can clawback funds at any time because there will no claim.

Related:

I stand corrected. You're right @smol-ninja - if no one can claim because the percentages don't add up, then it will always be possible to clawback deposited funds.

It sounds like we landed on a new solution that involves removing the check in the create function. I updated the OP. Can you confirm that it looks good to you?

Yes looks perfect. I see that you have removed the requirement for view function to compute address in advance. Do we not want that anymore? I have already implemented it in this PR but I can remove it.

The precompute utility is no longer strictly needed since there are no reverts. But it might still be useful to have it, so let's keep it.

Edit: changed my mind and removed the utilities

Added a big warning in case this happens and restricted access to the claims feature. I don't have an actual campaign to test this on but this is how it should look like:

Screenshot 2024-06-18 at 13 14 04

It's also possible to simply stop tracking this type of campaigns from the subgraphs/indexers.

Upside: campaign doesn't show up in the UI at all - no need for warnings
Downside: no UI for clawbacks

Thoughts?

This looks great. Easy to follow.

Downside: no UI for clawbacks

How about to show it if there are significant funds (>$10) available to be claw backed? Hide otherwise. Wdyt?

Hmm I think that's tough to do, as the subgraphs don't have access to live balances so filtering wouldn't work at that level. Also we don't have many campaigns where the amount would be so low (people usually fund it all at once right?).

I am in favor of keeping that error in the UI (instead of not indexing broken campaigns) on the basis that errors are guaranteed to be made. A creator who messed up would be very angry if they didn't have an easy way to recover their funds.

and yes the error looks great, kudos @razgraf

I do have a request: would it be too much for us to have a pure function in the contract that checks if the configuration of an MLT is sound? Math is tricky between browser and smart-contract, so validating that all the tranches add up correctly by making a call to check before the deployment would prevent possible fails frontend-side due to precision loss. @smol-ninja

@razgraf there is already a variable called TOTAL_PERCENTAGE which returns the sum of tranche's percentages. If its not 1e18 then claim would fail.

If you think a separate function would still be more useful, then please let me know. I can create a separate function areValidTranches() returns (bool) (or you can suggest a better name) that will return false if sum is not 100%.

I was thinking of a way to check pre-deployment, within the factory, not post-deployment within the campaign contract itself. e.g.

if(factory.isValid(tranches)){
   factory.deploy(tranches, ...)
}

Now I understand what you meant. Sounds like a good idea to me.

Created an issue: #356.