Making the clawback function permissionless after expiry?

[PaulRBerg]

Any reason for not making the clawback function permisionless (callable by anyone) after expiry?

cc @sablier-labs/solidity

[andreivladbrg]

just to confirm

you also mean to remove the to parameter as well:

airdrops/src/abstracts/SablierMerkleBase.sol

Line 155 in 2d1ba19

function clawback(address to, uint128 amount) external override onlyAdmin {

otherwise, there would be security issues.

---

my opinion is to not make it public, as one admin may want to leave the funds more time in the pool contract.
also, isn’t it contradictory with the escrow pool (APY) idea?

[PaulRBerg]

Well, we can either remove the to parameter, or we can introduce a new function that can only withdraw to the admin.

as one admin may want to leave the funds more time in the pool contract

I don't understand what you meant by this. Can you re-explain, please?

isn’t it contradictory with the escrow pool (APY) idea?

That idea involves writing a new contract. This proposal is specifically for the current contracts.

[andreivladbrg]

Well, we can either remove the to parameter, or we can introduce a new function that can only withdraw to the admin.

2 functions just for clawback, as simple as this functionality?

I don't understand what you meant by this. Can you re-explain, please?

let's assume this scenario: expiry date is 1st Feb, and the admin wants to claw back on 1st March. being public, anyone can claw back for him anytime between 1st Feb - 1st March. does it make sense now?

That idea involves writing a new contract. This proposal is specifically for the current contracts.

i know it is a new contract, but it would still inherit MerkleBase, where the clawback logic is. so, if we don't want to make changes to the current MerkleBase IMO we should keep it like this (we would have in MerkleLL MerkleLT and Instant a version of clawback, and on this new contract a different one)

[smol-ninja]

2 functions just for clawback, as simple as this functionality?

I wouldn't vote for having two functions for the same thing. An alias function will increase the gas cost at the expense of marginal difference in clawback UX.

[smol-ninja]

We will still need a msg.sender == admin check inside the function because it can also be called during the grace period (7 days after the first claim).

So, we can't completely make it permissionless.

Current implementation

function clawback(address to, uint128 amount) external override onlyAdmin {
    // Check: current timestamp is over the grace period and the campaign has not expired.
    if (_hasGracePeriodPassed() && !hasExpired()) {
        revert Errors.SablierMerkleBase_ClawbackNotAllowed({
            blockTimestamp: block.timestamp,
            expiration: EXPIRATION,
            firstClaimTime: _firstClaimTime
        });
    }

    TOKEN.safeTransfer(to, amount);

    emit Clawback(campaignAdmin, to, amount);
}

Proposed implemented

function clawback(uint128 amount) external override {
    // Check: current timestamp is over the grace period and the campaign has not expired.
    if (_hasGracePeriodPassed() && !hasExpired()) {
        revert Errors.SablierMerkleBase_ClawbackNotAllowed({
            blockTimestamp: block.timestamp,
            expiration: EXPIRATION,
            firstClaimTime: _firstClaimTime
        });
    }
    
    // Check: only the campaign admin can clawback funds during the grace period.
    if (!_hasGracePeriodPassed() && msg.sender != campaignAdmin) {
        revert Errors.SablierMerkleBase_CallerNotAdmin(msg.sender);
    }

    TOKEN.safeTransfer(campaignAdmin, amount);

    emit Clawback(campaignAdmin, amount);
}

I think the second function is lengthier, and more gas consuming as well.

[PaulRBerg]

Great point @smol-ninja, I forgot about the grace period.

Closing, this is not worth it.