jkoppel - claimByMerkleProof uses the distributor's domain, not the beneficiary's domain

[sherlock-admin2]

jkoppel

medium

claimByMerkleProof uses the distributor's domain, not the beneficiary's domain

Summary

claimByMerkleProof uses the distributor's domain, not the beneficiary's domain. It thus only works for beneficiary's on the same domain. While there are other ways to claim, they may be more expensive for the claimant

Vulnerability Detail

From claimByMerkleProof

    _verifyMembership(_getLeaf(_beneficiary, _total, domain), _proof);
    // effects
    uint256 claimedAmount = _executeClaim(_beneficiary, _total);

    // interactions
    _settleClaim(_beneficiary, _beneficiary, domain, claimedAmount);
  }

Note how it uses the variable domain, which is set to connext.domain() in the constructor. It thus only works if the beneficiary's domain is the same as the CrosschainDistributor.

https://github.com/sherlock-audit/2023-06-tokensoft/blob/main/contracts/contracts/claim/abstract/CrosschainMerkleDistributor.sol#L100

Impact

The impact is reduced because EOAs can still use claimBySignature, while xReceive can still be called on behalf of contracts. However, both of these take more gas, and using xReceive also requires paying the fees of Connext. Users will thus get less funds than if claimByMerkleProof worked correctly.

 * 2. `claimByMerkleProof` allows any address to claim funds on behalf of a beneficiary to the Connext domain and address specified in the merkle leaf by providing a merkle proof

https://github.com/sherlock-audit/2023-06-tokensoft/blob/main/contracts/contracts/claim/abstract/CrosschainMerkleDistributor.sol#L23

Code Snippet

Tool used

Manual Review

Recommendation

Fix to use beneficiaryDomain.

[jkoppel]

Escalate

The docs say in multiple places this function should be usable to claim funds for a beneficiary on whatever domain is specified in a Merkle leaf. But it cannot be. That makes this a valid issue.

Combined with #130, this bug means that there is no Tokensoft-provided way for smart contracts to claim their rewards cross-domain. So if #130 is a valid medium, then this should be as well.

[sherlock-admin2]

Escalate

The docs say in multiple places this function should be usable to claim funds for a beneficiary on whatever domain is specified in a Merkle leaf. But it cannot be. That makes this a valid issue.

Combined with #130, this bug means that there is no Tokensoft-provided way for smart contracts to claim their rewards cross-domain. So if #130 is a valid medium, then this should be as well.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[akshaysrivastav]

Hey #189 is a duplicate of this issue.

[Shogoki]

According to the sponsor (@cr-walker) this is intended behaviour of the contract.

As stated an EOA can still use the claim by signature or using the xcall to claim his tokens.

It is by design for contracts that they can only claim on the same domain.

[jkoppel]

Where was this stated? The documentation states very clearly that this is not the intended behavior, as cited above.

[Shogoki]

Where was this stated? The documentation states very clearly that this is not the intended behavior, as cited above.

Yep, I agree that it is not clearly stated in the documentation. It was just feedback from sponsor, that this works as intended.

This cannot be known, so this is can be a valid issue. I think it is on the verge between low and Medium though.

Tend more into low, because the only real impact would be that users have to spend more gas, because they have to use a different method.

[akshaysrivastav]

As per Sherlock rules, the contest readme is placed at the top of hierarchy of truth. So the behaviour mentioned in readme should be taken as the intended behaviour.

The contest readme explicitly mentions here that:

The beneficiaries (i.e. those people that will receive tokens in the airdrop) can be either EOAs or smart contracts, and can claim tokens with a merkle proof (crosschainTrancheVestingMerkle.claimByMerkleProof()) - EOA and smart contract: must receive tokens on address and chain specified in the merkle leaf

Moreover, a similar statement is present in code comments here which states:

claimByMerkleProof allows any address to claim funds on behalf of a beneficiary to the Connext domain and address specified in the merkle leaf by providing a merkle proof

Hence the protocol always intended to distributed tokens to the domain specified in the merkle tree, and this report demonstrates an otherwise behavior.

For these reasons the issue should be considered as a valid medium.

[Shogoki]

Hence the protocol always intended to distributed tokens to the domain specified in the merkle tree, and this report demonstrates an otherwise behavior.

I mean it still does that, it is not distributing tokens to another domain, that s not specified in the merkle tree.

As i stated in my last comment I think it is on the verge between Medium and Low.

[akshaysrivastav]

I mean it still does that, it is not distributing tokens to another domain, that s not specified in the merkle tree.

There seems to be a confusion here. The merkle tree can contain the domain of a different chain, different than the one on which distributor is deployed.

So, the Distributor could be deployed on Polygon and can contain a merkle leaf which states that 100 tokens should be distributed to Alice on Avalanche, and the contract must abide that and distributed those tokens to Alice on Avalanche. But currently that doesn't happen due to the reported bug.

[jkoppel]

I think it is on the verge between low and Medium though.

Tend more into low, because the only real impact would be that users have to spend more gas, because they have to use a different method.

This would be true, except that #130 means that, for smart contracts, TokenSoft provides no different method. (It is possible to write your own xCall, but if that matters for determining bug validity, then #130 shouldn't be a valid medium either. And, with the information provided in the contest, that doesn't work either because of #143.)

[hrishibhat]

@jkoppel let me know if I understand this correctly, if a smart contract was expecting a claim then claimByMerkleProof would not work. However, someone can call initialclaim via satellite on behalf of that beneficiary smart contract and still claim? is that correct?

[jkoppel]

@hrishibhat The first sentence, that claimByMerkleProof would not work, is correct. The second part is not correct because initiateClaim also does not work; see #130. There is no way to use TokenSoft code to let a smart contract on another chain claim an airdrop.

[hrishibhat]

@jkoppel yes agreed. But #130 is a different issue who's vulnerability was not identified here.

here is no way to use TokenSoft code to let a smart contract on another chain claim an airdrop.

I totally understand your point here. But there two issues need to be look at separately. From this issue's POV there claimByMerkleProof does not work but they can still claim via xreceive. Can be considered as a lost feature but the claim is still possible through another function in the same contract via the satellite feature. Hence the argument for low

However, #130 is an issue with initiateClaim which is its only job, rendering the contract useless. Breaking the core functionality of that contract with no other options within the contract. Hence this can be a fair medium

[jkoppel]

I don't see how that's a distinction. The only job of claimByMerkleProof is to let a contract or EOA claim token, regardless of what chain it's on. It does not do that, making it just as useless as initiateClaim except in the fraction of cases when the claimant is on the same chain.

On the other hand, initiateClaim is just a convenience function for xcall, which anyone can do. The Satellite contract in its entirety is unnecessary. It is in fact strictly worse than just calling xcall, because it performs an unnecessary check that costs gas.

Thus, I see stronger arguments for making this issue a medium and #130 a low than vice versa.

Even rejecting this argument, the combination of this issue with #130 is deadly and elevates its significance:

From the Sherlock docs:

if a Watson identifies an attack vector that combines multiple low's to cause significant loss/damage that would still be categorized as a valid medium/high.

https://docs.sherlock.xyz/audits/judging/judging

The combination of this issue with #130 means that contracts on another chain cannot claim tokens, period, except by using the workaround of calling xcall directly.

Because of this, I see no consistent position for marking this as a low but #130 as a medium. Either they should both be medium, or they should both be low.

[hrishibhat]

if Watson identifies an attack vector that combines multiple low's to cause significant loss/damage that would still be categorized as a valid medium/high.

@jkoppel Sir, I think you have misunderstood the rule. your submission does not identify the underlying problem which #130 does. So you cannot combine them together to make your issue valid. If it did identify I'd completely agree with your issue being a medium/high. From your submission POV one function does not work as intended in the contract but there is another function that can be used to do the same action.

But if we are to look at #130 separately, I can see your argument for it to be low, but the fact remains that the error there would render a contract useless and would need to be written. This affects the core functioning of that contract. And the workaround is outside of the contract.

[jkoppel]

Okay, I think we have common ground then.

[hrishibhat]

Result: Low Has duplicates Considering this issue a valid low based on the above discussion there are methods in the contracts that users can still claim with.

130 will be considered accordingly.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• jkoppel: rejected