jkoppel - PerAddressTrancheVestingMerkleDistributor.claim always reverts because it attempts to decode a bytes(0) as a Tranche[]. (And some discussion about issue duplication)

[sherlock-admin3]

jkoppel

medium

PerAddressTrancheVestingMerkleDistributor.claim always reverts because it attempts to decode a bytes(0) as a Tranche[]. (And some discussion about issue duplication)

Summary

PerAddressTrancheVestingMerkleDistributor.claim does the equivalent of calling abi.decode(new bytes(0), (Tranche[])), and thus always reverts

Vulnerability Detail

PerAddressTrancheVestingMerkleDistributor.claim passes in new bytes(0) instead of the encoded tranches, PerAddressTrancheVestingMerkle, which is implemented correctly. However, the recipient of this data attempts to decode it as Tranche[], and thus always fails.

Note that there are a grand total of 3 (!) issues, each of which independently cause PerAddressTrancheVestingMerkleDistributor.claim to always revert. They are (1) PerAddressTrancheVestingMerkleDistributor.claim passes along new bytes(0) instead of a valid encoding of a Tranche[], and (2) AdvancedDistributorInitializable ignores its data argument, and instead passes along new bytes(0), and (3) PerAddressTrancheVestingMerkleDistributor.claim does not includes the tranches when it checks the Merkle proof (and doesn't even take them as an argument), and thus always reverts before the method body is even entered. These are all separate issues.

Discussion of issue duplication

I am submitting 4 similar issues, two of which cause the same failure. I am also submitting a 5th issue which causes the same failure as the aforementioned 2. The Sherlock format encourages making separate reports for each issue, so I am doing so. I'd like to share my thoughts about which of these issues should be considered duplicates.

The current Sherlock rules state:

In case the same vulnerability appears across multiple places in different contracts, they can be considered duplicates. The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately.

It is also a general principle that, from the POV of each submitted issue, the rest of the contract should be assumed to work correctly.

Under those guidelines, I think it's pretty clear that this present issue should be considered separate from the Merkle proof issue that causes the same failure. Then the remaining question is how to merge the 4 issues involving erroneously passing in new bytes(0).

I would lean towards duplicating the three versions of that issue in PerAddressTrancheVestingMerkleDistributor.claim, PerAddressContinuousVestingMerkleDistributor.claim, and PerAddressContinuousVestingMerkle.claim, as those occur in very similar-looking functions, but leaving separate the version in AdvancedDistributorInitializable._executeClaim, which appears to be of independent origin (especially as the problem does not occur in AdvancedDistributor, a contract which is otherwise almost identical). The latter also impacts many future distributors. But there are good arguments for merging all four of these.

Impact

People cannot claim token from a PerAddressTrancheVestingMerkleDistributor

Code Snippet

From

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingMerkleDistributor.sol#L62

        uint256 claimedAmount = _executeClaim(beneficiary, totalAmount, new bytes(0));

This is intended to be forwarded to

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L37-L38

    function getVestedFraction(address beneficiary, uint256 time, bytes memory data) public view override returns (uint256) {
        Tranche[] memory tranches = abi.decode(data, (Tranche[]));

And hence always reverts

Tool used

Manual Review

Recommendation

Pass an encoding of the tranches along

Duplicate of #11