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:
-
block.timestamp
: all input parameters being same,block.timestamp
is the only value that guarantees to be cahnged over time. -
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:
- Exclude
tranchesWithPercentages
fromsalt
. So there is no need to revert increateMerkleLT
execution. - Store the total percentage sum as a
IMMUTABLE
inSablierV2MerkleLT
contract. 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 henceblock.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
.
- Valid point.
- 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:
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.