Allowing clawback after campaign has been created

[smol-ninja]

Comments from RustyRabbit (Cantina)

• Since aggregateAmount and recipientCount are not validated during the execution of createMerkle functions, they cannot be trusted in the emitted events.
• People tend to trust event data (blindly) as they assume it’s all validated.

An example scenario

1. A legitimate user creates an Airstream campaign.
2. A malicious actor creates another campaign with the same data except for the large value of expiration and aggregateAmount.
3. Since both the campaigns have the same initialAdmin, the Sablier UI will show both campaigns to the legitimate user.
4. The legitimate user can be tricked into funding the malicious stream with a large aggregateAmount. Since the expiration is very high, he wouldn’t even be able to clawback extra funds.

Comment from @razgraf :

• The chances are slim, as (1) they’d probably notice the amount being off (when dealing with such big amounts) and we also have the final step of Airstream creation include “fund” in the series of actions (so you can fund from the same page where you create the campaign).
• It’s quite hard to get this data from the IPFS file on-demand. That’d be the one way to circumvent it.

Alternate solution proposed by @smol-ninja:

• We can add a cooldown period of 24 hours. During that period, clawback can be called by the admin. it's not elegant but gives admin some time to adjust the deposit amount in case he has been tricked into depositing more.

[smol-ninja]

Conversation with RustyRabbit.

[smol-ninja]

Initial comments from the call with @razgraf and @andreivladbrg :

• Allow clawback until the first claim is made.
• After first claim, clawbacks period runs for further 7 days.

Implementation

• Add a new storage uint40 preClawbackStartTime.
• Every time claim is called, it would check the value of preClawbackStartTime and set it if not set.

function claim(....) {
  if (preClawbackStartTime == 0) {
    preClawbackStartTime = block.timestamp;
  }
  // --- snip ---
}

• Allow clawback if the following condition is met: block.timestamp <= preClawbackStartTime + 7 days

function clawback(....) {
  if (!hasExpired() || block.timestamp > preClawbackStartTime + 7 days) {
    revert Errors.SablierV2MerkleLockup_CampaignNotExpired(...);
  }
  // --- snip ---
}

• I also suggest to rename error SablierV2MerkleLockup_CampaignNotExpired to SablierV2MerkleLockup_ClawbackNotAllowed to reflect both the conditions.

Cons

• Since first claim will change the state, it will consume 22,100 additional gas. We can bring it down to 5000 by initialising preClawbackStartTime with a non-zero value such as 1.

Pros

• It can be considered as a solution to
  • https://github.com/cantinasec/review-sablier-3/issues/9
  • https://github.com/cantinasec/review-sablier-3/issues/3
  • https://github.com/cantinasec/review-sablier-3/issues/4
  • https://github.com/cantinasec/review-sablier-3/issues/6

tl;dr if user accidentally funds an incorrect amount or submits an invalid Merkle root, it provides them with a window to get the funds out of the contract.

We can brainstorm more on this today and finalize the solution tomorrow.

[PaulRBerg]

Regarding the name of the variable - let's name it as either firstClaimTime or gracePeriodStartTime.

Regarding the gas cost - let's stick with a default value of zero, for simplicity's sake.

[andreivladbrg]

Although I'd like to ask you this - do we really need the 7 days countdown? Why not just have a boolean flag that is toggled after the first claim is made? It shouldn't be possible to claim if the Merkle root is incorrect, right?

The rationale for a 7-day period after the first claim is based on the possibility of a malicious member making a random claim immediately after the campaign is deployed.

P.S. I don't see how the solution addresses the https://github.com/cantinasec/review-sablier-3/issues/3

Correct, it doesn't.

[PaulRBerg]

possibility of a malicious member making a random claim immediately after the campaign is deployed

random sounds like an oxymoron if the user is malicious. That claim would be purposefully done to block the campaign creator from clawing back the funds.

But tell me this - if we switch to simple CREATE instead of CREATE2, how can a malicious user even exist?

[smol-ninja]

It has nothing to do with CREATE2.

Any malicious actor can replay a campaign with a different set of data (such as high aggregateAmount, high expiration or a different Merkle root), and tricking the user into funding the malicious campaign. This is because the UI cannot differentiate the malicious one from the real other. And the funder would see both the campaigns in the dashboard.

@razgraf has suggested putting a warning in the UI if the admin and the campaign creator differ.

P.S. I don't see how the solution addresses the https://github.com/cantinasec/review-sablier-3/issues/3

Agree. For that Andrei has prepared a different response (its in security channel).

[PaulRBerg]

Got it, thanks for the additional color @smol-ninja. I understand the issue now, and I very much agree with adding a warning in the UI if there's a mismatch between the admin and the campaign creator.

You have the green light from me for implementing this post-first-claim grace period mechanism.