Abort function in Merkle vesting campaigns

[PaulRBerg]

UPDATE: we don't need to implement this. See my comment below.

Address Cantina finding 12 by implementing an abort function in the Merkle abstract contract.

The function should take an array of recipient addresses as inputs, and it should allow the campaign creator to effectively cancel the airstreams for particular users.

The _claim function should be modified accordingly to check for cancelations.

[PaulRBerg]

Note for @sablier-labs/frontend — we don't need to rush with implementing this functionality in the UI.

It's likely that it will be needed only in special circumstances, e.g., when there are conflicts between Sablier users.

So as far as the frontend is concerned, we can keep in on the back-burner for a while.

[smol-ninja]

The cancel function should also create stream for tokens already vested in case of ranged airstreams, and mark it as claimed.

[PaulRBerg]

Why do that when we can just add another mapping _canceled? The gas cost should be much lower.

Though I guess that by creating a stream and actually claiming the airstream, we would keep the gas cost for the _claim function unchanged.

[andreivladbrg]

I have an idea. What if we add both a _canceled flag and the timestamp at which it got canceled. This is helpful as we will also address this finding, i.e. we will be able to calculate the vesting amount based on a fixed range without depending on the block.timestamp, so we can do this:

uint128 claimableAmount = VestingMath.calculateLockupLinearStreamedAmount({
                depositedAmount: depositedAmount,
                blockTimestamp: canceledCampaignTimestamp // pass the timestamp at which it got canceled
                timestamps: timestamps,
                cliffTime: _cliffs[streamId],
                unlockAmounts: _unlockAmounts[streamId],
                withdrawnAmount: _streams[streamId].amounts.withdrawn
            });
// then just transfer the claimableAmount until that point 
TOKEN.transfer(recipient, claimableAmount);

return;

In this way, the recipient will also receive the funds streamed up to the cancellation point, thus both parties are happy.

[smol-ninja]

If the cancel does not create stream, then we will also have to store refunded amount in the storage, to ensure that when user claims, the stream is created for (airdrop amount - refunded amount). This will also make claim execution look ugly since the original amount is the part of the merkle tree.

[smol-ninja]

To @andreivladbrg, interesting idea but

• the user will still be passing original amount in the claim function (merkle data)

• more SLOAD

imo if we create stream or transfer streamed tokens directly to the recipient without creating stream when user cancels and then mark it as claimed, we wont have to introduce any additional logic or storage variable at all. Wdyt?

[PaulRBerg]

we will also have to store refunded amount in the storage

Ooh, right.

In this case, yeah, creating a stream is the easiest way.

However, I'm reluctant to call this function cancel because it would be misleading w.r.t the cancel logic in Lockup.

How about dismiss, abort, revoke, or terminate? Just to create some differentiation.

[smol-ninja]

"terminate" sounds good to me.

Instead of creating the stream, I'd recommend to transfer the tokens directly to the recipient, since the vesting would come to an end when sender calls this function (similar to #37).

[PaulRBerg]

That makes sense, but it may create trouble for data analytics for airstreams. A portion of the airstream data would need to be sourced from Lockup, and another portion from Merkle.

[smol-ninja]

I see. Wouldn't it be the same problem in #37 as well?

[PaulRBerg]

Yeah, fair enough, let's go with the immediate transfer option.

[smol-ninja]

We will have to go with @andreivladbrg's proposal of storing termination timestamp in the storage. And then in claim, calculate the amount vested until termination timestamp and transfer it to the recipient.

Otherwise if recipient is a blacklisted address, sender wont be able to terminate it.

[andreivladbrg]

Sheesh, nice one @smol-ninja, you are correct. I will pursue with this implementation.

[andreivladbrg]

Re terminology, my preferences are abort and revoke, as "terminate" makes me think that the stream is already created and then an action is being taken. In contrast, the terms I mentioned seem clearer to me, as they suggest that an action is not taken but rather prevented. wdyt?

[smol-ninja]

terminate" makes me think that the stream is already created and then an action is being taken

Isn't that what is happening here? A vesting schedule is already created and then an action (cancel) is being taken on it.

[andreivladbrg]

A vesting schedule is already created and then an action (cancel) is being taken on it.

IMO the vesting schedule is created when the stream is created. Anytime before this, I would refer to it as "schedule setup"

[smol-ninja]

IMO the vesting schedule is created when the stream is created. Anytime before this, I would refer to it as "schedule setup"

I disagree. Vesting schedule is created as soon as the campaign is launched in case of ranged airstreams. End users don't care about the internal logic. For them and also for the campaign creators, streams vesting schedule are synchronous for all users. But I am not strongly against abort and revoke as well. Feel free to pick whatever you like.

[PaulRBerg]

Agree with @smol-ninja, but I also don't think there's any meaningful semantic difference between abort, revoke, and terminate.

Let's go with abort.

Otherwise if recipient is a blacklisted address, sender wont be able to terminate it.

Good catch. This risk was first reported in the 2023 audit performed by Cantina: https://github.com/cantinasec/review-sablier/issues/11

[andreivladbrg]

Question re implementation. Should we add some relevant checks? Or the admin should be allowed to abort a stream no matter what?

The checks I have in mind are these (lmk if you have other in mind):

1. if (hasExpired()) --> revert
2. if (!STREAM_CANCELABLE) --> revert
   • if the streams are set to be non cancelable, the problem mentioned in the finding, does not exist
3. if (endTime < blockTimestamp) --> revert
4. if (startTime > blockTimestamp) --> revert

The time checks are important because, in the case of a stream being aborted after the end time (for the linear Merkle contract), the VestingMath.calculateLockupLinearStreamedAmount function would return 0 due to this check. However, in the case of the tranched model, it would correctly return the deposit amount.

Relevant test for Vesting math

function test_VestingMathFunctions_Tranched_BlockTime_Greater() public view {
    uint40 time = _defaultParams.tranches[_defaultParams.tranches.length - 1].timestamp + 10_000;
    uint128 amount = VestingMath.calculateLockupTranchedStreamedAmount(time, _defaultParams.tranches);
    console.log("amount:", amount);

    console.log("is equal to deposit: ", amount == defaults.DEPOSIT_AMOUNT());
}

function test_VestingMathFunctions_Linear_BlockTime_Greater() public view {
    uint40 time = _defaultParams.tranches[_defaultParams.tranches.length - 1].timestamp + 10_000;
    uint128 amount = VestingMath.calculateLockupLinearStreamedAmount(
        defaults.DEPOSIT_AMOUNT(),
        time,
        _defaultParams.createWithTimestamps.timestamps,
        _defaultParams.cliffTime,
        _defaultParams.unlockAmounts,
        0 // pass 0 for the withadrawn amount
    );

    console.log("amount:", amount);
    console.log("is equal zero ", amount == 0);
}

function test_VestingMathFunctions_Tranched_BlockTime_Less() public view {
    uint40 time = _defaultParams.createWithTimestamps.timestamps.start - 10_000;
    uint128 amount = VestingMath.calculateLockupTranchedStreamedAmount(time, _defaultParams.tranches);
    console.log("amount:", amount);

    console.log("is equal zero: ", amount == 0);
}

function test_VestingMathFunctions_Linear_BlockTime_Less() public {
    uint40 time = _defaultParams.createWithTimestamps.timestamps.start - 10_000;

    _defaultParams.cliffTime = 0;
    _defaultParams.createWithTimestamps.totalAmount =
        _defaultParams.createWithTimestamps.totalAmount + _defaultParams.unlockAmounts.cliff;
    _defaultParams.unlockAmounts.cliff = 0;

    uint128 amount = VestingMath.calculateLockupLinearStreamedAmount(
        defaults.DEPOSIT_AMOUNT(),
        time,
        _defaultParams.createWithTimestamps.timestamps,
        _defaultParams.cliffTime,
        _defaultParams.unlockAmounts,
        0 //
    );
    console.log("amount:", amount);

    console.log("is equal zero: ", amount == 0);
}

The issue with the end-time check for the tranche Merkle is that there’s no straightforward way to calculate the end time, apart from calling _calculateStartTimeAndTranches and then using tranches[tranches.length - 1].timestamp. This approach isn’t very gas-efficient. Should we keep it as is and let the admin accepts the additional costs, or should we modify the factory contract to include the end time?

@sablier-labs/solidity RFF

[smol-ninja]

if (hasExpired()) --> revert

It should not revert if campaign has expired. Stream end time can be greater than expiry timestamp.

if (!STREAM_CANCELABLE) --> revert

Agree.

if (endTime < blockTimestamp) -> revert

I think it should revert but as you said its complicated to calculate end time. What if we don't revert and let sender receive 0 amount (since there is no amount to be canceled anymore).

if (startTime > blockTimestamp) --> revert

We should let sender abort it even if the vesting has not begun. I don't see a reason why it should revert. We allow sender to cancel a Lockup stream even if it has not started right?

[andreivladbrg]

It should not revert if campaign has expired. Stream end time can be greater than expiry timestamp

Yes, that’s correct—the end time can be greater than the expiry timestamp. However, that’s not the issue here. It simply doesn’t make sense to abort something that can’t be claimed

I think it should revert but as you said its complicated to calculate end time. What if we don't revert and let sender receive 0 amount (since there is no amount to be canceled anymore).

There would be a discrepancy between MerkleLL and MerkleLT. MerkleLL would receive 0, while MerkleLT would receive the deposit amount. Refer to the tests test_VestingMathFunctions_Tranched_BlockTime_Greater vs. test_VestingMathFunctions_Linear_BlockTime_Greater in the details block.

We should let sender abort it even if the vesting has not begun. I don't see a reason why it should revert. We allow sender to cancel a Lockup stream even if it has not started right?

i agree.

[andreivladbrg]

We should let sender abort it even if the vesting has not begun. I don't see a reason why it should revert. We allow sender to cancel a Lockup stream even if it has not started right?

Actually, I don’t agree. It’s not the same because, in the cancel function, we have this check that isn’t included in the VestingMath library. So, for extra safety, I believe we should ensure that the abort time is between the start and end time

[smol-ninja]

It simply doesn’t make sense to abort something that can’t be claimed

Good point. I missed that.

MerkleLL would receive 0, while MerkleLT would receive the deposit amount

I see. Shouldn't the Vesting Math function return deposit amount for both LL and LT if block time > end time? We should change that in vesting math logic right?

Actually, I don’t agree. It’s not the same because, in the cancel function, we have this check that isn’t included in the VestingMath library. So, for extra safety, I believe we should ensure that the abort time is between the start and end time

If the abort time < start time, why do we need to interact with VestingMath library? Shouldn't we return 0 locally in the abort function?

[andreivladbrg]

I see. Shouldn't the Vesting Math function return deposit amount for both LL and LT if block time > end time? We should change that in vesting math logic right?

We could do that, but it would mean duplicating logic across all functions, and I think it’s fine as we’ve also made the change to include the blockTimestamp as a parameter. Now, the VestingMath library can be used as-is (see this)

also (since it is a public library), would the linking be made automatically? 🤔

If the abort time < start time, why do we need to interact with VestingMath library? Shouldn't we return 0 locally in the abort function?

I think there’s a misunderstanding. I’ve implemented the functionality in the feat/abort branch. Please see commit 95e1b48 for what I mean.

Now that we have an initial implementation, I’m starting to feel like the change might be too big for such an unlikely edge case

[smol-ninja]

also (since it is a public library), would the linking be made automatically?

What do you mean?

I think there’s a misunderstanding

After looking at the implementation, indeed there is a misunderstanding. The implementation looks good.

I’m starting to feel like the change might be too big for such an unlikely edge case

Whats the big change? The implementation seems not so big.

[andreivladbrg]

What do you mean?

we have deployable libraries. calling VestingMath.foo() doesn't inline the logic into the Merkle bytecode, so it needs to be know where to take that logic from. does it make sense?

After looking at the implementation, indeed there is a misunderstanding. The implementation looks good.

glad to hear

Whats the big change? The implementation seems not so big.

well it is the biggest change for this audit, it changes the logic of how a claim works.

[smol-ninja]

we have deployable libraries. calling VestingMath.foo() doesn't inline the logic into the Merkle bytecode, so it needs to be know where to take that logic from. does it make sense?

Got it. So to answer your question, when we deploy the contracts, foundry should automatically links the new libraries with the contract.

[PaulRBerg]

I just realized something important about finding 12.

We don't need to implement this abort function because ..

When the campaign creator detects an ill-intentioned user for whom they want to cancel the airdrop ..

They can just claim the airdrop stream NFT for them, and then cancel as usual.

So I'm closing this issue.

Sorry for the sunk cost @andreivladbrg @smol-ninja.

[andreivladbrg]

We don't need to implement this abort function because ..

that's right

Sorry for the sunk cost @andreivladbrg @smol-ninja.

no worries, the fix was very ugly, so this is even better we can achieve it without any other problems