Modify "claim" function in MerkleLockup to charge an ETH fee

[PaulRBerg]

Rationale

Monetize Airstreams. See https://github.com/sablier-labs/company-discussions/discussions/72.

I have considered alternative designs whereby the fee is relayed to a Sablier pot, but the added gas cost does not seem worth it. The gas cost paid by users may be higher than the fee itself. Instead of making N transfers, we can just retroactively 'sweep up' the fees from the campaigns that have generated significant amounts.

Spec

• Modify the claim function
  • Add the payable modifier
  • Check that msg.value is at least sablierFee
• Introduce a new variable sablierFee of type uint256 and a setter setSablierFee that is admin-gated
• Implement a withdrawFees function (check out this example)

[smol-ninja]

Do we want to cap sablierFee to any amount like how we do it for broker and protocol fee?

We should not because the fee is in the dollar value and it can vary on each chain.

[PaulRBerg]

Yeah

[smol-ninja]

Why do we want to have UD60x18 type for sablier fee? IMO, we should use uint256 for this. It's not a percentage, and the native token amount is always represented as uint256 when creating a tx. Even when the call is made by a contract, the amount will have to be passed as uint256 in msg.value. Therefore, I do not see any value in using UD60x18 for sablier fee.

[PaulRBerg]

Correct. We should use uint256 for sablierFee because it's a minimum threshold, not a percentage.

[smol-ninja]

Given that we now have sablier admin as well as the merkle lockup admin, should we replace merkle lockup admin with campaignCreator or owner to differentiate between the two?

• admin mens Sablier admin
• owner means campaign owner

Any other ideas?

Here is my implementation:

• Factory inherits Adminable and sets sablier admin as the admin of the factory contract.
• When create functions are called, sablier admin and sablier fee stored in the factory contract are passed to the constructor.
• Sablier admin and sablier fee are then stored as SABLIER and SABLIER_FEE constants in the newly deployed merkle lockup contracts. Note that merkle lockup also inherits Adminable but to store campaign creator as the admin.

In the source contract, the naming difference is pretty clear and may not lead to confusion to the users.

• Factory has admin (inherited from Adminable) which means sablier admin
• MerkleBase has admin (inherited from Adminable) representing campaign creators and SABLIER (set through constructor) representing sablier admin.

But in tests, we use users.admin to represent the campaign creators as well as the sablier admin. Thats where it cause the confusion. Or either I can introduce campaignCreator user in the User struct to represent the campaign creator or we can consider renaming admin to campaignCreator in the merkle lockup contracts.

cc @sablier-labs/solidity

[PaulRBerg]

I suggest walking on the path of maximum explicitness and going with campaignCreator or campaignOwner in the MerkleLockup contracts. I don't care so much about the raw word as much as about using the 'campaign' prefix to clearly differentiate between the Sablier admin and the campaign creator.

stored as SABLIER and SABLIER_FEE constants

Why constants? They should be editable.

[smol-ninja]

Why constants? They should be editable

They are editable in the factory contract but not in the Merkle contracts. The reason is that for a new campaign, the creator knows the fee in advance, and it ensures that these fees won't be changed for that particular campaign. The cons of making them editable in merkle Lockup is that admin can change fee in the middle of an active campaign.

[PaulRBerg]

OK, makes sense, I will have another think during the internal audit.

[andreivladbrg]

Why don’t we simplify the implementation by allowing the factory contract to be the caller of the fee action in the Merkle campaigns? This way, we avoid having two "admins"

// MerkleBase.sol
address public FACTORY;
constructor() {
    FACTORY = msg.sender;
}

function withdrawFees(address to) public {
    // Check: the caller is the factory contract.
    if (msg.sender != FACTORY) {
        revert Errors.CallerNotFactory(FACTORY, msg.sender);
    }
    
    // --snip--
}

// MerkleFactory.sol
function withdrawFees(address to, ISablierMerkleBase merkle) onlyAdmin {
     merkle.withdrawFees(to);
}

@sablier-labs/solidity wdyt?

[andreivladbrg]

or like uniswap v3 does (our Merkle campaign is equivalent to their pool)

https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L111-L115

but i believe the version from above is better

[smol-ninja]

I like @andreivladbrg's approach. The benefit is that, we can keep admin instead of campaignOwner in merkle lockup and inherit admin related functionalities from Adminable contract. Otherwise I had to re-write all functions from Adminable into MerkleBase contract using the campaign owner terminologies.

[andreivladbrg]

The cons of making them editable in merkle Lockup is that admin can change fee in the middle of an active campaign

I just saw this, and I believe this is a feature, not a con. As mentioned in the comment here, I think we should make them editable.

To make an analogy, it would be like activating the protocol fee (with the version implemented in withdraw) and only benefiting from the streams created after it was activated. We don't do this, but we can still gain fees even if the stream was created prior to activation (ofc, if they still have funds to withdraw).

[smol-ninja]

Good analogy between protocol fee and clam fee. I'd like to share my views on why they are not the same.

1. Streams generally have longer life spans compared to claiming airstream which is a one-time event. That's why I agree having an editable protocol fee is beneficial. On the other hand, since claiming is a singular event, keeping the claim fee fixed during the lifetime of a campaign offers peace of mind to both campaign creators and claimers.

2. Secondly, protocol fees are calculated as a percentage, and there's a cap at 10%. This prevents any possible misuse as the maximum extractable value is predefined. Whereas, claim fees are absolute amounts (fixed in ETH) and could potentially be set to any value, which might expose users to unexpected high fees if editable.

3. For all the campaigns that were created before the claim fee is turned on, we can still earn from them through protocol fee.

[PaulRBerg]

Great idea to allow only the factory to call the withdrawFees functions! It alleviates a whole set of problems.

However, I agree with Shub that we should not make the fee editable for airstreams. The only thing I would add is that we may want to charge different campaigns differently.

[andreivladbrg]

Thanks for the fast answer.

fixed during the lifetime of a campaign offers peace of mind to both campaign creators and claimers

Well, I don’t think campaign creators are affected by this, as they’ve already paid the "fee" by funding the campaign.
As for claimers, when it comes to airdrops, it’s basically free money, so it seems fair to charge a small fee for the good UX and experience we offer when claiming something that’s free

protocol fees are calculated as a percentage, and there's a cap at 10%. This prevents any possible misuse as the maximum extractable value is predefined. Whereas, claim fees are absolute amounts (fixed in ETH) and could potentially be set to any value, which might expose users to unexpected high fees if editable

That's correct, but I am sorry, I don't understand why this argument is relevant to immutability of the fees? It can still be applied to new campaigns that are created after the fee is set in the factory contract.

For all the campaigns that were created before the claim fee is turned on, we can still earn from them through protocol fee

this is partially correct, as for MerkleInstant we wouldn't earn anything as no stream is created to withdraw from.

The only thing I would add is that we may want to charge different campaigns differently.

how is this going to be possible?

---

the fixed fee made me think, what if we charge a percentage as well? an example: sablierFee would be 10% of tx.gasprice, wdyt?

[andreivladbrg]

Since this is a product design choice, I kindly ask you @sablier-labs/engineers to provide your input:

Should claim fees be charged on:
1️⃣ All campaigns that have been created through that factory contract
2️⃣ Only the campaigns created after the fee is activated

[PaulRBerg]

I don’t think campaign creators are affected by this

They absolutely are.

Campaign creators are inextricably linked with their claimers. If their claimers cannot claim because of a very high fee set by Sablier, they will go complain to the campaign creator, not us. They will think that the campaign creator set the high fee.

it seems fair to charge a small fee for the good UX

Yes. A small, predictable, fixed fee. Not one that can be set to 1 ETH overnight.

I don't understand why this argument is relevant to immutability of the fees? It can still be applied to new campaigns that are created after the fee is set in the factory contract.

I don't understand why you don't understand?

New campaigns are irrelevant to an existing campaign. The point is for all claimers in a campaign to know transparently ahead of time how much they will pay. If we can change the fee, some will pay 0.001 ETH, others 0.1 ETH, others 1 ETH, etc.

The only thing I would add is that we may want to charge different campaigns differently.

how is this going to be possible?

You're right, never mind that.

what if we charge a percentage as well? an example: sablierFee would be 10% of tx.gasprice, wdyt?

Gas prices are haphazard. They are outside our control and we shouldn't charge our users like this.

[PaulRBerg]

I suggest opening a discussion with the possible designs laid out clearly.

[andreivladbrg]

Yes. A small, predictable, fixed fee. Not one that can be set to 1 ETH overnight

We can hardcode a constant MAX_FEE_IN_WEI = 0.01e18 // 0.01 ETH

If their claimers cannot claim because of a very high fee set by Sablier, they will go complain to the campaign creator, not us

Agreed, but as mentioned above, we can hardcode a max fee, so the "very high fee" issue won’t be a problem.

Also, I want to point out that we’ve received complaints in the past for mainnet campaigns where the gas fees for the tx were higher than what participants actually received in USD (we can't know in advance what amount will the creators give to recipients). So, this issue exists regardless of whether we have a fee or not. It’s something we can’t predict.

I don't understand why you don't understand?

please see again my comment: "why this argument is relevant to immutability of the fees? It can still be applied to new campaigns that are created after the fee is set in the factory contract." - the idea is that the argument exists outside immutability of the fee per se in one campaign

The argument that the fee is fixed and not a percentage applies whether the change affects all existing and future campaigns or only future campaigns. For example, for token X, campaigns X1, X2, and X3 were created. For X1, the fee is 0, but for X2 and X3, the fee > 0. X2 and X3 would still suffer of "which might expose users to unexpected high fees if editable" Please lmk if that clarifies my point.

New campaigns are irrelevant to an existing campaign. The point is for all claimers in a campaign to know transparently ahead of time how much they will pay. If we can change the fee

They will know for sure in advance, as we will give a note in the app that we are charging a fee. So this isn't a problem.

some will pay 0.001 ETH, others 0.1 ETH, others 1 ETH, etc.

(not talking about the fee amounts, as they might not be precise)
As mentioned in my initial comment, I see this as a feature, not a con. It could also lead to faster claims for recipients, as it introduces an unexpected behavior, which could ultimately generate more traffic/users for us.

Gas prices are haphazard. They are outside our control and we shouldn't charge our users like this

you are right, it is better to be fixed

[andreivladbrg]

I suggest opening a discussion with the possible designs laid out clearly.

#1039

[andreivladbrg]

The native tokens on non-ETH EVMs can be highly volatile. Thus, hard coding it to anything can limit us to how much we can charge when the price of native token is very low. Whereas the same arguement does not apply to the protocol fee since the fee is in percentage.

it can be passed at the deployment time, i don't find this as a problem.

/// Factory
uint256 public immutable MAX_FEE;

// depending on the chain, we pass 0.01 ETH, 5 MATIC, etc.
constructor(uint256 maxFee) {
     MAX_FEE = maxFee;
}

Whereas the same arguement does not apply to the protocol fee since the fee is in percentage.

the percentage is in the token set in the stream, as @PaulRBerg said:

Gas prices are haphazard. They are outside our control and we shouldn't charge our users like this

[smol-ninja]

it can be passed at the deployment time

Let's say you pass 10 MATIC during the deployment time (which is equivalent to $1). And then the price fell by 80% so now 10 MATIC is $0.2. You won't be able to charge users $1 anymore. Alternatively, let's say the price goes up to 10x. So now the max is $10. Contrary to your claim that fixing would eliminate the "very high fee" issue, when the price shoots up to 10x or 100x, the very high fee issue comes back.

What matters for us is the dollar value (also mentioned in the original discussion).

[PaulRBerg]

To @andreivladbrg:

his issue exists regardless of whether we have a fee or not. It’s something we can’t predict.

There are nuances; it's possible that without the Sablier fee, the gas fee is not greater than the USD value of the stream, but when the Sablier fee is added, the total fee is greater.

They will know for sure in advance, as we will give a note in the app that we are charging a fee. So this isn't a problem.

It is a problem because they will one day see that there are no fees, and they will be left with the impression that there are no fees. Also, the campaign creator might advertise the airdrop as free to claim.

It could also lead to faster claims for recipients, as it introduces an unexpected behavior, which could ultimately generate more traffic/users for us.

I very much doubt that that will happen. If users don't care about claiming the airdrop, it means it's a low-value airdrop. The incentive to claim because Sablier might enable fees at some point in the feature is weak.

Please lmk if that clarifies my point.

I'm afraid it doesn't. I didn't understand your follow-up. Could you please reformulate it?

[PaulRBerg]

In any case. I agree with what @maxdesalle said here. It's simply better to not be able to change the fee retroactively.

To answer @smol-ninja's concern about the volatility of the token.

It's a relatively minor issue. 80% corrections don't happen that often (thankfully!). Airdrop claim windows last a few months on average (AFAIK).

So hard-coding is fine.

[PaulRBerg]

There have been some misunderstandings above, which I have clarified on a call with @andreivladbrg.

I don't think it's worth re-explaining them in writing given that we have reached an agreement in #1039.