Alternative solution to "claimAndExecute"

[andreivladbrg]

Initially posted here

instead of dedicated functions in each merkle contract like claimAndDoSomething, what if we:

• add a general-purpose execute function with a target whitelisting system
• inherit the Batch contract so it can call batch with: claim + execute

@sablier-labs/evm

[smol-ninja]

Can you provide a PoC please similar to #1336 (comment), so there is no misunderstanding between what you are proposing and what I understand.

[andreivladbrg]

while writing the POC i realized that none of these options (neither yours nor mine) would work in practice (at least with our version of staking), because the merkle contract would be considered the staker for the internal logic of the staking contract. see here and here.
so we would need to implement more functions from the staking contract (unstake and claimRewards) in the merkle campaigns as well.

which i no longer think is a good idea, because it’s not a great UX for airdrop recipients to be unable to view their “stake weight” as theirs, and it adds much more logic for it to work in practice.

also, wouldn’t all this “claimAndStake” be fixed with AA and native batch?

[smol-ninja]

When i was writing the proposal I considered the following function (which we don't have in our staking contract but should have in the future):

function stakeERC20Token(uint256 poolId, uint128 amount, address onBehalfOf) external;

Aave has a similar feature where you can lend on behalf of another user and that user will be able to withdraw collateral (because its his now). Now if we assume this and we insist prospects to design their stake function in this manner, then the problem is solved, right?

My idea was to implement claimAndExecute as a general purpose function which can be used to a wide variety of use cases such as claim and swap, claim and stake to validators (requested by Aztec) etc.

What do you think now?

---

also, wouldn’t all this “claimAndStake” be fixed with AA and native batch?

I guess yes but I dont think AA has seen any adoption rate. I still dont see anyone using it including Uniswap and Aave.

[andreivladbrg]

When i was writing the proposal I considered the following function (which we don't have in our staking contract but should have in the future):

i see. this is why i mentioned in my comment “at least with our version of staking,” which is also mentioned in the OP

but with the function that stakes on behalf of someone, you are right, this would work. and yes, it would be useful to have it in staking

Now if we assume this and we insist prospects to design their stake function in this manner, then the problem is solved, right?

i don’t know the data out there, but it depends on whether the staking contract already exists and is deployed or not, so that they can add more features

My idea was to implement claimAndExecute as a general purpose function which can be used to a wide variety of use cases such as claim and swap, claim and stake to validators (requested by Aztec) etc

i understand this, but the point i made was that it won’t work in all cases

---

re the alternative solution i suggested, this is the POC (both stake + approve must be allowed selectors):

contract SablierMerkleBase is Batch {
    mapping(bytes4 => bool) public allowedSelectors;

    function execute(address target, bytes calldata data) external {
        bytes4 selector = bytes4(data[:4]);
        if (!allowedSelectors[selector]) revert InvalidSelector(selector);
        (bool success,) = target.call(data);
        if (!success) revert CallFailed(target, data);
    }
}

and the batch function must be used like this:

// Step 1: claim to the campaign itself
bytes memory claimToCall = abi.encodeCall(campaign.claimTo, (index, amount, address(campaign), merkleProof)); 
// Step 2: call approve on token from campaign
bytes memory approveCall =
    abi.encodeCall(campaign.execute, (address(token), abi.encodeCall(IERC20.approve, (address(stakeContract), amount))));
// Step 3: call execute to call stake
bytes memory stakeCall =
    abi.encodeCall(campaign.execute, (address(stakeContract), abi.encodeCall(stakeContract.stake, (params));

// Build the batch
bytes[] memory batchData = new bytes[](3);
batchData[0] = claimToCall;
batchData[1] = approveCall;
batchData[2] = stakeCall;

// Run claim + stake
campaign.batch(batchData);

note: no need to reduce the allowance at the end as it is done automatically by erc20 contract

this version is fully “general-purpose,” and i believe it’s better in terms of backwards compatibility with indexers (from both parties airdrops and staking protocols), since it uses the currently available function claimTo, and also reduces code (no claim function in each campaign contract)
the con is that it does a transfer from the campaign to the campaign (self), but i believe it’s fine since gas costs are cheap nowadays and claiming already has a fee ($2). if you do the diff, percentage-wise it’s negligible.

[smol-ninja]

it would be useful to have it in staking

I agree. And I was planning to add it to the next release of staking (we need to integrate the first release in the UI first) which can be done when we release next release of Airdrops.

but it depends on whether the staking contract already exists and is deployed or not, so that they can add more features

Yup. But this is true for all cases. If they do not allow staking on behalf of other user then its impossible to integrate claim into it.

---

Thanks for providing the PoC and helping me understand what you were saying. In your implementation, you are assuming that:

1. User only wants to claim and stake
2. The stake function has a very specific signature with same parameters

The solution I provided is very generic (think of it like Lockup hooks). It can support many number of applications. Anyone can build anything such as claim-and-lend, claim-and-swap etc with it without us having to make any changes to Airdrops protocol.

no need to reduce the allowance at the end as it is done automatically by erc20 contract

The assumption is that target wants to transfer 100% of the tokens. In the general purpose claimAndExecute, I assumed target may only need a portion of the tokens and therefore it would require TOKEN.approve(target, 0); in the same tx. I don't know in what use cases but from the security point of view, its still god. For ex. if the target does not transfer tokens, it can do so later which will become a security concern for us.

---

Given my explanations above, what do you think now about the solution I proposed in #1336 (comment)?

[andreivladbrg]

as discussed on the call, the problem with the suggested version is that a malicious recipient can max approve a target and steal all funds from the campaign

[PaulRBerg]

@andreivladbrg can you please tag @sablier-labs/evm in the OP so that we receive email notifications

[andreivladbrg]

sure, updated the OP

[PaulRBerg]

@andreivladbrg let's discuss this idea when we meet in person

[smol-ninja]

As discussed with Andrei, it makes sense to have claimAndExecute part of a new merkle contract. For example, user who has requested this feature didn't want their recipients to use claim but not use claim-and-stake. If we add claimAndExecute to Merkle Instant then this will allow recipients to claim without going through staking (or lending or whatever).

So, it makes sense to have a new Merkle contract with only claimAndExecute and no other claim or claimTo or claimViaSig.

[PaulRBerg]

I've read this discussion and I agree with your conclusion. Could you close this, close issue #1336, and open a new issue with a clearer specification?

[andreivladbrg]

Closing in favor of #1356

[andreivladbrg]

I am re-opening this because while working on this, I came across a technical limitation with the design from #1356.

The Problem

Storing a single target address and targetData bytes doesn't account for recipients having different airdrop amounts. If the integrated protocol's deposit/stake function requires an amount parameter encoded in the calldata:

1. Recipient has less than the encoded amount: the call fails because the campaign only approves the recipient's actual allocation, but the target tries to pull the larger encoded amount.

2. Recipient has more than the encoded amount: only the encoded amount gets deposited, and the remaining tokens stay stuck in the campaign contract (never transferred to the recipient).

Possible Solutions

Workarounds within current design:

• The target protocol's function doesn't require an amount param - which is a big limitation
• Campaign creators deploy a wrapper contract between the campaign and the target protocol that handles the amounts
  • this wrapper contract will be the target in our campaign contract

Alternative designs:

• Include targetData per-recipient in the Merkle tree (unique calldata per recipient)
• Encode the minimum amount (off all recipients) in targetData and then, for recipients with airdropped amounts greater than this, transfer the tokens as @smol-ninja shown in his comment here

@sablier-labs/evm wdyt?

[smol-ninja]

thats my first point in "Alternative designs:"

I thought you mean to include it in the merkle tree. But I suggested to include target and targetData (or payload) as arguments of claimAndExecute function.

Include targetData per-recipient in the Merkle tree (unique calldata per recipient)

[andreivladbrg]

I thought you mean to include it in the merkle tree.

yeah, but if we include them in the merkle tree or not, it would be a requirement to pass them as params in the claim function - so thats the same thing

on the discussion about whether to include them in the merkle tree or not, i believe that including them (since they are no longer set up by the campaign creator) would be an extra security measure, as one recipient could otherwise pass another recipient’s target and data, even tho only their amount is approved

the trust assumption is between the campaign creator and the recipients, not between individual recipients :))

[smol-ninja]

one recipient could otherwise pass another recipient’s target and data, even tho only their amount is approved

How is that a problem? Its the same thing as saying what if a recipient passes another recipient's address in claimTo function. What matters is that only a recipient can call this function his behalf. So, there is no trust assumption here. If I am eligible, you are letting me do whatever I want with my claim amount. Also, the approval is set to 0 at the end of the function which is a good safety mechanism.

The idea behind setting TARGET as an immutable variable was to restrict recipients to interact only with what has been whitelisted by the campaign admin (there were requests where tokens were non-transferrable except when staking).

So I believe, TARGET should still be an immutable variable. Having said that, what if we let campaign creator set function signature?

For example, campaign creator can set the signature of stake(address, uint256) function. And then in claimAndExecute, we take bytes memory payload as an argument, which is nothing but function data. So, in this case, payload would be 0xAddressAmount, and then we create target data as function signature + 0xAddressAmount and use it in TARGET.call.

This will allow:

• Campaign creator to restrict what function that can interact with on which contract
• Recipients can call any data on that function

If the amount is higher than claimed amount, the call will revert which is good. If the amount is less, the call succeed and the remaining will be sent to the recipient's EOA

[andreivladbrg]

good points Shub, i like this:

• save target address in state
• save function selector in state
• have just bytes arguments as param in claim
• abi encode the selector with the arguments

tho, i want to mention that if we let recipients pass any target address and any data, they could simply pass the airdropped token address and then max-approve a random address they control, and basically steal all funds. this is why i made the security point, does it make sense?
but by saving the target and the selector in state, we don’t have this problem

closing

[smol-ninja]

does it make sense?

Yes. And good observation.