search

sherlock-audit / 2024-05-tokensoft-distributor-contracts-update-judging

3 stars 2 forks source link

aman - The last index should be checked for fraction denominator #46

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

aman

high

The last index should be checked for fraction denominator

Summary

The comments on getVestedFraction states that the if last tranche the vested fraction will be fraction denominator. However it is not done in case of PerAddress

Vulnerability Detail

It is not enforce in code that the last vesting must be the fraction denominator. and in other cases like with the continuous vesting we explictly check that if end of vesting has passed return the fraction denominator. 

2024-05-tokensoft-distributor-contracts-update/contracts/packages/hardhat/contracts/claim/factory/PerAddressContinuousVestingInitializable.sol:30
30:     function getVestedFraction(
31:         address beneficiary,
32:         uint256 time, // time is in seconds past the epoch (e.g. block.timestamp)
33:         bytes memory data
34:     ) public view override returns (uint256) {
35:         (uint256 start, uint256 cliff, uint256 end) = abi.decode(data, (uint256, uint256, uint256));
36: 
37:         uint256 delayedTime = time - getFairDelayTime(beneficiary);
38:         // no tokens are vested
39:         if (delayedTime <= cliff) {
40:             return 0;
41:         }
42: 
43:         // all tokens are vested
44:         if (delayedTime >= end) { <--------------
45:             return fractionDenominator;
46:         }
47: 
48:         // some tokens are vested
49:         return (fractionDenominator * (delayedTime - start)) / (end - start);
50:     }

but in case of Tranche Vesting we are not returning the fraction denominator but instead returns vested fraction :

2024-05-tokensoft-distributor-contracts-update/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol:37
37:     function getVestedFraction(address beneficiary, uint256 time, bytes memory data) public view override returns (uint256) {
39:         Tranche[] memory tranches =  abi.decode(data, (Tranche[]));
40: 
41:         uint256 delay = getFairDelayTime(beneficiary);
42:         for (uint256 i = tranches.length; i != 0;) {
43:             unchecked {
44:                 --i;
45:             }
46:             if (time - delay > tranches[i].time) {
47:                 return tranches[i].vestedFraction;
48:             }
49:         }
50:         return 0; // will return 0 here
51:     }

To Identify the Claimable amount at specific time we use following formula :

        uint256 claimable = (record.total * getVestedFraction(beneficiary, block.timestamp, data)) / fractionDenominator;

if getVestedFraction return fractionDenominator then the user will be able to claim all token.

Impact

The claimable amount will not be correct as there is a high chance of that last vestedFraction != fractionDenominator. Because we allow full claim after vesting end time but the amount return will not be full amount.

Code Snippet

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

Tool used

Manual Review

Recommendation

change the code to following :

      if (time - delay > tranches[i].time && i==tranches.length-1) { // it means the last one and time has passed
          return fractionDenominator;
        }
        else if(time - delay > tranches[i].time) { 
            return tranches[i].vestedFraction;
        }
amankakar commented 5 months ago

@MxAxM, please share the rejection response.

MxAxM commented 5 months ago

@MxAxM, please share the rejection response.

user should be able to claim full amount after end-time otherwise calculate vested amount, what's wrong with it ?

amankakar commented 5 months ago

Hi @MxAxM , Basically the Issue here is that the code claim that the last vested Fraction should be calculated on basis of fractionDenominator. However in the code it is not done like it and the last tranche data is not validated. if last vestedFraction=8000 and fractionDenominator=10000 the formula : uint256 claimable = (record.total * getVestedFraction(beneficiary, block.timestamp, data)) / fractionDenominator; Will not calculated the correct amount. we have only one tranche with following data :

tranches:{
time : block.timestamp+2;
vestedFraction: 8000
}

getVestedFraction will return 8000 event if it is the last vested , lets assume total amount is record.total=1e18
$1e18 * 8000 / 10000 =800000000000000000(0.8 ether)$ So here the user was suppose to claim the total=1e18(1 ether);

MxAxM commented 5 months ago

Hi @MxAxM , Basically the Issue here is that the code claim that the last vested Fraction should be calculated on basis of fractionDenominator. However in the code it is not done like it and the last tranche data is not validated. if last vestedFraction=8000 and fractionDenominator=10000 the formula : uint256 claimable = (record.total * getVestedFraction(beneficiary, block.timestamp, data)) / fractionDenominator; Will not calculated the correct amount. we have only one tranche with following data :

tranches:{
time : block.timestamp+2;
vestedFraction: 8000
}

getVestedFraction will return 8000 event if it is the last vested , lets assume total amount is record.total=1e18 1e18∗8000/10000=800000000000000000(0.8ether) So here the user was suppose to claim the total=1e18(1 ether);

Can you provide a permalink to comment you're mentioning ?

amankakar commented 5 months ago

please have a look at here : https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-amankakar/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L31-L53.

MxAxM commented 5 months ago

please have a look at here : https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-amankakar/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L31-L53.

It says : After the last tranche time, the vested fraction will be the fraction denominator, it means It returns denominator if last tranche end time is passed not during that period

amankakar commented 5 months ago

please have a look at here : https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-amankakar/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L31-L53.

It says : After the last tranche time, the vested fraction will be the fraction denominator, it means It returns denominator if last tranche end time is passed not during that period

Exactly that is my point : How can we identify that the last tranche time has passed at first place. lets understand this with example data: Alice create Tranche Vesting with following data for Bob as a beneficiary: record.total= 1 ether; with the following vesting time: { time: block.timesstamp+1; vestedFraction:8000; }

Alice after block.timesstamp+100 calls the claim function(At this timestamp Alice would be able to claim all the tokens). The claim calls the getVestedFraction to identify how much token can be claimed at this point. The code Snippet which is used to calculate the vestedFraction:

    for (uint256 i = tranches.length; i != 0; ) {
      unchecked {
        --i;
      }
//  * After the last tranche time, the vested fraction will be the fraction denominator.
// it 
      if (time - delay > tranches[i].time ) { // Here the Time for last tranche has already passed , but it still return the vestedFraction
        return tranches[i].vestedFraction;
      }
    }

This function which calculates the claimable amount is getClaimableAmount :

        uint256 claimable = (record.total * getVestedFraction(beneficiary, block.timestamp, data)) / fractionDenominator;

Now put our values and vestedFraction into above calcualtion: $claimable= 1e18*8000/10000=> 0.8$ . it return 0.8 ethers but it should have returned 1 ether for which the bob is entitled to receive. Now the state Variable record.claimed=0.8 ether. The Bob will never be able to claim the 0.2 ether because the claim function will always return 0.8 ether which he has already claimed.

        return record.claimed >= claimable // 
            ? 0 // no more tokens to claim
            : claimable - record.claimed; // claim all available tokens

Not as a proof but to let you know that the sponsor has already conformed it in discord private chat. you can share it with them.

MxAxM commented 5 months ago

please have a look at here : https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-amankakar/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L31-L53.

It says : After the last tranche time, the vested fraction will be the fraction denominator, it means It returns denominator if last tranche end time is passed not during that period

Exactly that is my point : How can we identify that the last tranche time has passed at first place. lets understand this with example data: Alice create Tranche Vesting with following data for Bob as a beneficiary: record.total= 1 ether; with the following vesting time: { time: block.timesstamp+1; vestedFraction:8000; }

Alice after block.timesstamp+100 calls the claim function(At this timestamp Alice would be able to claim all the tokens). The claim calls the getVestedFraction to identify how much token can be claimed at this point. The code Snippet which is used to calculate the vestedFraction:

    for (uint256 i = tranches.length; i != 0; ) {
      unchecked {
        --i;
      }
//    * After the last tranche time, the vested fraction will be the fraction denominator.
// it 
      if (time - delay > tranches[i].time ) { // Here the Time for last tranche has already passed , but it still return the vestedFraction
        return tranches[i].vestedFraction;
      }
    }

This function which calculates the claimable amount is getClaimableAmount :

        uint256 claimable = (record.total * getVestedFraction(beneficiary, block.timestamp, data)) / fractionDenominator;

Now put our values and vestedFraction into above calcualtion: claimable=1e18∗8000/10000=>0.8 . it return 0.8 ethers but it should have returned 1 ether for which the bob is entitled to receive. Now the state Variable record.claimed=0.8 ether. The Bob will never be able to claim the 0.2 ether because the claim function will always return 0.8 ether which he has already claimed.

        return record.claimed >= claimable // 
            ? 0 // no more tokens to claim
            : claimable - record.claimed; // claim all available tokens

Not as a proof but to let you know that the sponsor has already conformed it in discord private chat. you can share it with them.

In the scenario you described after end time user vested fraction would be denominator, since he claimed 0.8 ether so he would receive 0.2 ether, am I right or misunderstood something ?

amankakar commented 5 months ago

After the vested end time the user would be able to claim 1 ether which is total amount for vesting. Due to issue which I have described he will only be able to claim 0.8 ether. The remaining 0.2 ether will be locked in the contract.

On Sun, Jun 9, 2024, 4:43 PM xMxAxMx @.***> wrote:

please have a look at here : https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-amankakar/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L31-L53 .

It says : After the last tranche time, the vested fraction will be the fraction denominator, it means It returns denominator if last tranche end time is passed not during that period

Exactly that is my point : How can we identify that the last tranche time has passed at first place. lets understand this with example data: Alice create Tranche Vesting with following data for Bob as a beneficiary: record.total= 1 ether; with the following vesting time: { time: block.timesstamp+1; vestedFraction:8000; }

Alice after block.timesstamp+100 calls the claim function(At this timestamp Alice would be able to claim all the tokens). The claim calls the getVestedFraction to identify how much token can be claimed at this point. The code Snippet which is used to calculate the vestedFraction:

for (uint256 i = tranches.length; i != 0; ) {
  unchecked {
    --i;
  }//     * After the last tranche time, the vested fraction will be the fraction denominator.// it
  if (time - delay > tranches[i].time ) { // Here the Time for last tranche has already passed , but it still return the vestedFraction
    return tranches[i].vestedFraction;
  }
}

This function which calculates the claimable amount is getClaimableAmount :

    uint256 claimable = (record.total * getVestedFraction(beneficiary, block.timestamp, data)) / fractionDenominator;

Now put our values and vestedFraction into above calcualtion: claimable=1e18∗8000/10000=>0.8 . it return 0.8 ethers but it should have returned 1 ether for which the bob is entitled to receive. Now the state Variable record.claimed=0.8 ether. The Bob will never be able to claim the 0.2 ether because the claim function will always return 0.8 ether which he has already claimed.

    return record.claimed >= claimable //
        ? 0 // no more tokens to claim
        : claimable - record.claimed; // claim all available tokens

Not as a proof but to let you know that the sponsor has already conformed it in discord private chat. you can share it with them.

In the scenario you described after end time user vested fraction would be denominator, since he claimed 0.8 ether so he would receive 0.2 ether, am I right or misunderstood something ?

— Reply to this email directly, view it on GitHub https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-judging/issues/46#issuecomment-2156458266, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJD5ESN7IFYMSIT4JYRRHVTZGQ5VPAVCNFSM6AAAAABI26YVZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWGQ2TQMRWGY . You are receiving this because you commented.Message ID: <sherlock-audit/2024-05-tokensoft-distributor-contracts-update-judging/issues/46/2156458266 @github.com>

amankakar commented 5 months ago

Hi @MxAxM , Thanks for your support and patience all the way in this debate. I have written a code POC which will help you to understand what i am trying to explain. please add this test case file to project and test it.

import "forge-std/Test.sol";

contract TestClaimable is Test {

    struct Tranche {
        uint128 time;
        uint128 vestedFraction;
    }

    uint256 totalVested = 1 ether; // total amount a user can claim
    uint256 claimed = 0;
    uint256 fractionDenominator = 10000; // Fee denominator

    function setUp() external {}

    function testClaimableAmount() external {
        bytes memory tranches = encodeTranches();
        vm.warp(block.timestamp); // before the time it will return 0.
        uint256 amountBeforeTime = getClaimableAmount(address(0), tranches);
        assertEq(amountBeforeTime, 0);

        vm.warp(block.timestamp + 2 days); // the time has passed
        // @audit : At this point the user must be able to claim All the vested token in this tranche
        uint256 amountAfterEndTime = getClaimableAmount(address(0), tranches);
        // but he only receive 0.8 ether
        assertEq(amountAfterEndTime, 0.8 ether);
        // Assume here user has claim 0.8 ether so we store this value in claim state veraible
        claimed += amountAfterEndTime;

        vm.warp(block.timestamp + 2 days); // here again tries to claim the 0.2 ether left from 1 ether
        uint256 triesToClaimLeftEther = getClaimableAmount(
            address(0),
            tranches
        );
        // will revert because the amount is not correct
        vm.expectRevert();
        assertEq(triesToClaimLeftEther, 0.2 ether);
        assertEq(triesToClaimLeftEther, 0);
    }

    function getClaimableAmount(
        address beneficiary,
        bytes memory data
    ) public view returns (uint256) {
        uint256 claimable = (totalVested *
            getVestedFraction(beneficiary, block.timestamp, data)) /
            fractionDenominator;
        return
            claimed >= claimable
                ? 0 // no more tokens to claim
                : claimable - claimed; // claim all available tokens
    }

    function getVestedFraction(
        address beneficiary,
        uint256 time,
        bytes memory data
    ) public view returns (uint256) {
        Tranche[] memory tranches = decodeTranches(data);
        uint256 delay = 0;
        for (uint256 i = tranches.length; i != 0; ) {
            unchecked {
                --i;
            }

            if (time - delay > tranches[i].time) {
                return tranches[i].vestedFraction;
            }
        }
        return 0; // will return 0 here
    }

    function encodeTranches() public view returns (bytes memory) {
        Tranche[] memory tranchesArray = new Tranche[](1);
        tranchesArray[0] = Tranche({
            time: uint128(block.timestamp + 10),
            vestedFraction: 8000
        });
        return abi.encode(tranchesArray);
    }

    function decodeTranches(
        bytes memory data
    ) public pure returns (Tranche[] memory) {
        Tranche[] memory tranches = abi.decode(data, (Tranche[]));
        return tranches;
    }
}
MxAxM commented 5 months ago

After the vested end time the user would be able to claim 1 ether which is total amount for vesting. Due to issue which I have described he will only be able to claim 0.8 ether. The remaining 0.2 ether will be locked in the contract. … On Sun, Jun 9, 2024, 4:43 PM xMxAxMx @.**> wrote: please have a look at here : https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-amankakar/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L31-L53 . It says : After the last tranche time, the vested fraction will be the fraction denominator, it means It returns denominator if last tranche end time is passed not during that period Exactly that is my point : How can we identify that the last tranche time has passed at first place. lets understand this with example data: Alice create Tranche Vesting with following data for Bob as a beneficiary: record.total= 1 ether; with the following vesting time: { time: block.timesstamp+1; vestedFraction:8000; } Alice after block.timesstamp+100 calls the claim function(At this timestamp Alice would be able to claim all the tokens). The claim calls the getVestedFraction to identify how much token can be claimed at this point. The code Snippet which is used to calculate the vestedFraction: for (uint256 i = tranches.length; i != 0; ) { unchecked { --i; }// After the last tranche time, the vested fraction will be the fraction denominator.// it if (time - delay > tranches[i].time ) { // Here the Time for last tranche has already passed , but it still return the vestedFraction return tranches[i].vestedFraction; } } This function which calculates the claimable amount is getClaimableAmount : uint256 claimable = (record.total * getVestedFraction(beneficiary, block.timestamp, data)) / fractionDenominator; Now put our values and vestedFraction into above calcualtion: claimable=1e18∗8000/10000=>0.8 . it return 0.8 ethers but it should have returned 1 ether for which the bob is entitled to receive. Now the state Variable record.claimed=0.8 ether. The Bob will never be able to claim the 0.2 ether because the claim function will always return 0.8 ether which he has already claimed. return record.claimed >= claimable // ? 0 // no more tokens to claim : claimable - record.claimed; // claim all available tokens Not as a proof but to let you know that the sponsor has already conformed it in discord private chat. you can share it with them. In the scenario you described after end time user vested fraction would be denominator, since he claimed 0.8 ether so he would receive 0.2 ether, am I right or misunderstood something ? — Reply to this email directly, view it on GitHub <#46 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJD5ESN7IFYMSIT4JYRRHVTZGQ5VPAVCNFSM6AAAAABI26YVZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWGQ2TQMRWGY . You are receiving this because you commented.Message ID: <sherlock-audit/2024-05-tokensoft-distributor-contracts-update-judging/issues/46/2156458266 @github.com>

vestedFraction means percentage of tokens that is vesting and fraction denominator means 100% percentage of vested amount

amankakar commented 5 months ago

After the vested end time the user would be able to claim 1 ether which is total amount for vesting. Due to issue which I have described he will only be able to claim 0.8 ether. The remaining 0.2 ether will be locked in the contract. … On Sun, Jun 9, 2024, 4:43 PM xMxAxMx @.**> wrote: please have a look at here : https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-amankakar/blob/main/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L31-L53 . It says : After the last tranche time, the vested fraction will be the fraction denominator, it means It returns denominator if last tranche end time is passed not during that period Exactly that is my point : How can we identify that the last tranche time has passed at first place. lets understand this with example data: Alice create Tranche Vesting with following data for Bob as a beneficiary: record.total= 1 ether; with the following vesting time: { time: block.timesstamp+1; vestedFraction:8000; } Alice after block.timesstamp+100 calls the claim function(At this timestamp Alice would be able to claim all the tokens). The claim calls the getVestedFraction to identify how much token can be claimed at this point. The code Snippet which is used to calculate the vestedFraction: for (uint256 i = tranches.length; i != 0; ) { unchecked { --i; }// After the last tranche time, the vested fraction will be the fraction denominator.// it if (time - delay > tranches[i].time ) { // Here the Time for last tranche has already passed , but it still return the vestedFraction return tranches[i].vestedFraction; } } This function which calculates the claimable amount is getClaimableAmount : uint256 claimable = (record.total * getVestedFraction(beneficiary, block.timestamp, data)) / fractionDenominator; Now put our values and vestedFraction into above calcualtion: claimable=1e18∗8000/10000=>0.8 . it return 0.8 ethers but it should have returned 1 ether for which the bob is entitled to receive. Now the state Variable record.claimed=0.8 ether. The Bob will never be able to claim the 0.2 ether because the claim function will always return 0.8 ether which he has already claimed. return record.claimed >= claimable // ? 0 // no more tokens to claim : claimable - record.claimed; // claim all available tokens Not as a proof but to let you know that the sponsor has already conformed it in discord private chat. you can share it with them. In the scenario you described after end time user vested fraction would be denominator, since he claimed 0.8 ether so he would receive 0.2 ether, am I right or misunderstood something ? — Reply to this email directly, view it on GitHub <#46 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJD5ESN7IFYMSIT4JYRRHVTZGQ5VPAVCNFSM6AAAAABI26YVZCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWGQ2TQMRWGY . You are receiving this because you commented.Message ID: </issues/46/2156458266 @github.com>

I think you misunderstood difference between vestedFraction and fraction denominator, vestedFraction means total amount of tokens that is vesting and fraction denominator means 100% percentage of vested amount

Have you checked the POC?.

The vestedFraction by definition is not totalAmount it is the fraction of totalAmount that is available to be claimed at given time. For Total Amount that would be vested we have this struct

struct DistributionRecord {
  bool initialized; // has the claim record been initialized
  uint120 total; // total token quantity claimable
  uint120 claimed; // token quantity already claimed
}

/it value initializes when we create new Vesting inside : _initializeDistributionRecord().
        records[beneficiary] = DistributionRecord(true, totalAmount, records[beneficiary].claimed);
MxAxM commented 5 months ago

So vestedFraction should be equal to fraction denominator.

Owner sets vestedFraction at setTranches function, we consider admin would set this value correctly

amankakar commented 5 months ago

setTranches could never be called because it is not there for reference please run this test case .

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.21;

import "forge-std/Test.sol";

import "../../contracts/claim/factory/TrancheVestingMerkleDistributorFactory.sol";
import "../../contracts/claim/factory/TrancheVestingMerkleDistributor.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

import "../../contracts/claim/factory/PerAddressTrancheVestingMerkleDistributor.sol";
import "../../contracts/claim/factory/PerAddressTrancheVestingMerkleDistributorFactory.sol";

import "../../contracts/utilities/Registry.sol";

contract TrancheVestingMerkleDistributorFactoryTest is Test {
    TrancheVestingMerkleDistributor implementation;
    TrancheVestingMerkleDistributor clone;
    TrancheVestingMerkleDistributorFactory factory;
    PerAddressTrancheVestingMerkleDistributorFactory factory1;

    Tranche[] tranches = [Tranche(1, 8000)];

    PerAddressTrancheVestingMerkleDistributor implementation1;

    function setUp() public {
        implementation = new TrancheVestingMerkleDistributor();
        factory = new TrancheVestingMerkleDistributorFactory(
            address(implementation)
        );
    }

    function test_SetUp() public {
        assertEq(address(factory.getImplementation()), address(implementation));
    }

    function test_DeployDistributor() public {
        clone = factory.deployDistributor(
            IERC20(token),
            1000,
            "uri",
            tranches,
            bytes32(0),
            0,
            address(this),
            0
        );

        assertEq(clone.owner(), address(this));
        assertEq(clone.getSweepRecipient(), address(this));
    }

    function test_PredictDistributorAddress() public {
        uint256 total = 1000;
        string memory uri = "uri";
        bytes32 merkleRoot = bytes32(0);
        uint160 maxDelayTime = 0;
        address owner = address(this);
        uint256 nonce = 1;

        address nextCloneAddress = factory.predictDistributorAddress(
            token,
            total,
            uri,
            tranches,
            merkleRoot,
            maxDelayTime,
            owner,
            nonce
        );
        TrancheVestingMerkleDistributor nextClone = factory.deployDistributor(
            token,
            total,
            uri,
            tranches,
            merkleRoot,
            maxDelayTime,
            owner,
            nonce
        );

        assertEq(nextCloneAddress, address(nextClone));
    }

    function testClaimRevert() external {
        PerAddressTrancheVestingMerkleDistributor distributor = new PerAddressTrancheVestingMerkleDistributor();
        distributor.initialize(
            IERC20(token),
            1000,
            "uri",
            bytes32(
                0xf32ce147ef6c8e7a07fbe3ce1ae1aef2405a59b50db2a8ede14f4e75bfe7d949
            ),
            10,
            address(this)
        );
        factory1 = new PerAddressTrancheVestingMerkleDistributorFactory(
            address(distributor)
        );
        implementation1 = factory1.deployDistributor(
            IERC20(token),
            1000,
            "uri",
            bytes32(
                0xf32ce147ef6c8e7a07fbe3ce1ae1aef2405a59b50db2a8ede14f4e75bfe7d949
            ),
            10,
            address(this),
            1
        );
        implementation1.setToken(IERC20(token));
        bytes32[] memory proof = new bytes32[](1);
        token.transfer(address(implementation1), 10e18);
        proof[0] = bytes32(
            0xa4170e52dc35c1127b67e1c2f5466fce9673e61b44e077b7023f7c994d55c5dd
        );
        try implementation1.claim(
            1,
            address(0x70997970C51812dc3A010C7d01b50e0d17dc79C8),
            5000000000000000000000,
            proof
        ){} catch{}
        // Tranche[] memory _tranches = new 
        uint256 frc = implementation1.setTranches(tranches);

    }

}

Please add this test file inside foundry test cases and run with command : forge test --mt testClaimRevert . it will gives you error : Member "setTranches" not found or not visible after argument-dependent lookup in contract PerAddressTrancheVestingMerkleDistributor.

MxAxM commented 5 months ago

setTranches could never be called because it is not there for reference please run this test case .

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.21;

import "forge-std/Test.sol";

import "../../contracts/claim/factory/TrancheVestingMerkleDistributorFactory.sol";
import "../../contracts/claim/factory/TrancheVestingMerkleDistributor.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

import "../../contracts/claim/factory/PerAddressTrancheVestingMerkleDistributor.sol";
import "../../contracts/claim/factory/PerAddressTrancheVestingMerkleDistributorFactory.sol";

import "../../contracts/utilities/Registry.sol";

contract TrancheVestingMerkleDistributorFactoryTest is Test {
  TrancheVestingMerkleDistributor implementation;
  TrancheVestingMerkleDistributor clone;
  TrancheVestingMerkleDistributorFactory factory;
  PerAddressTrancheVestingMerkleDistributorFactory factory1;

  Tranche[] tranches = [Tranche(1, 8000)];

  PerAddressTrancheVestingMerkleDistributor implementation1;

  function setUp() public {
      implementation = new TrancheVestingMerkleDistributor();
      factory = new TrancheVestingMerkleDistributorFactory(
          address(implementation)
      );
  }

  function test_SetUp() public {
      assertEq(address(factory.getImplementation()), address(implementation));
  }

  function test_DeployDistributor() public {
      clone = factory.deployDistributor(
          IERC20(token),
          1000,
          "uri",
          tranches,
          bytes32(0),
          0,
          address(this),
          0
      );

      assertEq(clone.owner(), address(this));
      assertEq(clone.getSweepRecipient(), address(this));
  }

  function test_PredictDistributorAddress() public {
      uint256 total = 1000;
      string memory uri = "uri";
      bytes32 merkleRoot = bytes32(0);
      uint160 maxDelayTime = 0;
      address owner = address(this);
      uint256 nonce = 1;

      address nextCloneAddress = factory.predictDistributorAddress(
          token,
          total,
          uri,
          tranches,
          merkleRoot,
          maxDelayTime,
          owner,
          nonce
      );
      TrancheVestingMerkleDistributor nextClone = factory.deployDistributor(
          token,
          total,
          uri,
          tranches,
          merkleRoot,
          maxDelayTime,
          owner,
          nonce
      );

      assertEq(nextCloneAddress, address(nextClone));
  }

  function testClaimRevert() external {
      PerAddressTrancheVestingMerkleDistributor distributor = new PerAddressTrancheVestingMerkleDistributor();
      distributor.initialize(
          IERC20(token),
          1000,
          "uri",
          bytes32(
              0xf32ce147ef6c8e7a07fbe3ce1ae1aef2405a59b50db2a8ede14f4e75bfe7d949
          ),
          10,
          address(this)
      );
      factory1 = new PerAddressTrancheVestingMerkleDistributorFactory(
          address(distributor)
      );
      implementation1 = factory1.deployDistributor(
          IERC20(token),
          1000,
          "uri",
          bytes32(
              0xf32ce147ef6c8e7a07fbe3ce1ae1aef2405a59b50db2a8ede14f4e75bfe7d949
          ),
          10,
          address(this),
          1
      );
      implementation1.setToken(IERC20(token));
      bytes32[] memory proof = new bytes32[](1);
      token.transfer(address(implementation1), 10e18);
      proof[0] = bytes32(
          0xa4170e52dc35c1127b67e1c2f5466fce9673e61b44e077b7023f7c994d55c5dd
      );
      try implementation1.claim(
          1,
          address(0x70997970C51812dc3A010C7d01b50e0d17dc79C8),
          5000000000000000000000,
          proof
      ){} catch{}
      // Tranche[] memory _tranches = new 
      uint256 frc = implementation1.setTranches(tranches);

  }

}

Please add this test file inside foundry test cases and run with command : forge test --mt testClaimRevert . it will gives you error : Member "setTranches" not found or not visible after argument-dependent lookup in contract PerAddressTrancheVestingMerkleDistributor.

Do you have any idea why this test reverts ? I think there is a problem with your PoC since it doesn't recognize setTranches

amankakar commented 5 months ago

setTranches could never be called because it is not there for reference please run this test case .

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.21;

import "forge-std/Test.sol";

import "../../contracts/claim/factory/TrancheVestingMerkleDistributorFactory.sol";
import "../../contracts/claim/factory/TrancheVestingMerkleDistributor.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

import "../../contracts/claim/factory/PerAddressTrancheVestingMerkleDistributor.sol";
import "../../contracts/claim/factory/PerAddressTrancheVestingMerkleDistributorFactory.sol";

import "../../contracts/utilities/Registry.sol";

contract TrancheVestingMerkleDistributorFactoryTest is Test {
    TrancheVestingMerkleDistributor implementation;
    TrancheVestingMerkleDistributor clone;
    TrancheVestingMerkleDistributorFactory factory;
    PerAddressTrancheVestingMerkleDistributorFactory factory1;

    Tranche[] tranches = [Tranche(1, 8000)];

    PerAddressTrancheVestingMerkleDistributor implementation1;

    function setUp() public {
        implementation = new TrancheVestingMerkleDistributor();
        factory = new TrancheVestingMerkleDistributorFactory(
            address(implementation)
        );
    }

    function test_SetUp() public {
        assertEq(address(factory.getImplementation()), address(implementation));
    }

    function test_DeployDistributor() public {
        clone = factory.deployDistributor(
            IERC20(token),
            1000,
            "uri",
            tranches,
            bytes32(0),
            0,
            address(this),
            0
        );

        assertEq(clone.owner(), address(this));
        assertEq(clone.getSweepRecipient(), address(this));
    }

    function test_PredictDistributorAddress() public {
        uint256 total = 1000;
        string memory uri = "uri";
        bytes32 merkleRoot = bytes32(0);
        uint160 maxDelayTime = 0;
        address owner = address(this);
        uint256 nonce = 1;

        address nextCloneAddress = factory.predictDistributorAddress(
            token,
            total,
            uri,
            tranches,
            merkleRoot,
            maxDelayTime,
            owner,
            nonce
        );
        TrancheVestingMerkleDistributor nextClone = factory.deployDistributor(
            token,
            total,
            uri,
            tranches,
            merkleRoot,
            maxDelayTime,
            owner,
            nonce
        );

        assertEq(nextCloneAddress, address(nextClone));
    }

    function testClaimRevert() external {
        PerAddressTrancheVestingMerkleDistributor distributor = new PerAddressTrancheVestingMerkleDistributor();
        distributor.initialize(
            IERC20(token),
            1000,
            "uri",
            bytes32(
                0xf32ce147ef6c8e7a07fbe3ce1ae1aef2405a59b50db2a8ede14f4e75bfe7d949
            ),
            10,
            address(this)
        );
        factory1 = new PerAddressTrancheVestingMerkleDistributorFactory(
            address(distributor)
        );
        implementation1 = factory1.deployDistributor(
            IERC20(token),
            1000,
            "uri",
            bytes32(
                0xf32ce147ef6c8e7a07fbe3ce1ae1aef2405a59b50db2a8ede14f4e75bfe7d949
            ),
            10,
            address(this),
            1
        );
        implementation1.setToken(IERC20(token));
        bytes32[] memory proof = new bytes32[](1);
        token.transfer(address(implementation1), 10e18);
        proof[0] = bytes32(
            0xa4170e52dc35c1127b67e1c2f5466fce9673e61b44e077b7023f7c994d55c5dd
        );
        try implementation1.claim(
            1,
            address(0x70997970C51812dc3A010C7d01b50e0d17dc79C8),
            5000000000000000000000,
            proof
        ){} catch{}
        // Tranche[] memory _tranches = new 
        uint256 frc = implementation1.setTranches(tranches);

    }

}

Please add this test file inside foundry test cases and run with command : forge test --mt testClaimRevert . it will gives you error : Member "setTranches" not found or not visible after argument-dependent lookup in contract PerAddressTrancheVestingMerkleDistributor.

Do you have any idea why this test reverts ? I think there is a problem with your PoC since it recognize setTranches

I don't think that becuase the claim function is working which is inside PerAddressTrancheVestingMerkle and the set Tranche function is in not there lets have look at contract link with other contracts:

PerAddressTrancheVestingMerkle -> PerAddressTrancheVesting -> AdvancedDistributor -> Distributor . these contract does not have any link with the TrancheVestingMerkleDistributor:TrancheVestingInitializable:setTranches. In contract TrancheVestingInitializable inside _setTranche function they also have this check require(lastVestedFraction == fractionDenominator, "last tranche must vest all tokens"); which is exactly my point in this finding . if PerAddressTrancheVestingMerkle some how have access to this _setTranche function then i would not have submitted this finding.

That why it says that setTranche is not found . my test case is correct. i can also share the diagram if you require.

sherlock-admin3 commented 5 months ago

Escalate

On basis of above points discussed with lead judge.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

MxAxM commented 5 months ago

PerAddressTrancheVestingMerkle works different that other TrancheVestings, tranche array is passed as parameter to claim => _executeClaim > super._executeClaim => getClaimableAmount => getVestedFraction, then getVestedFraction decodes data and use tranche array which contains vestedFraction so it doesn't need to setTranches manually

In TrancheVesting there is a Tranche[] array that can be set globally by setTranches function but in PerAddressTrancheVestingMerkle it's passed as argument

amankakar commented 5 months ago

PerAddressTrancheVestingMerkle works different that other TrancheVestings, tranche array is passed as parameter to claim => _executeClaim > super.executeClaim => getClaimableAmount => getVestedFraction, then getVestedFraction decodes data and use tranche array which contains vestedFraction

So vestedFraction should be equal to fraction denominator.

Owner sets vestedFraction at setTranches function, we consider admin would set this value correctly

The above quoted point was shared by you.

it doesn't need to setTranches manually this was not my point you shared it that if tranche is wrong then owner can set it via setTranche function. and my points is that the tranche last vestedFraction is not checked that why we need to check if it is last tranche than instead of relaying on passed tranche data we must return fractionDenominator . please at least diacuss it with the sponsor Because he has agreed with me on this one in our chat . Because the admin is only trusted for sweep the funds and also add / remove tokens as well as per readMe we can relay on passed tranche data via sender.

MxAxM commented 5 months ago

PerAddressTrancheVestingMerkle works different that other TrancheVestings, tranche array is passed as parameter to claim => _executeClaim > super.executeClaim => getClaimableAmount => getVestedFraction, then getVestedFraction decodes data and use tranche array which contains vestedFraction

So vestedFraction should be equal to fraction denominator.

Owner sets vestedFraction at setTranches function, we consider admin would set this value correctly

The above quoted point was shared by you.

it doesn't need to setTranches manually this was not my point you shared it that if tranche is wrong then owner can set it via setTranche function. and my points is that the tranche last vestedFraction is not checked that why we need to check if it is last tranche than instead of relaying on passed tranche data we must return fractionDenominator . please at least diacuss it with the sponsor Because he has agreed with me on this one in our chat . Because the admin is only trusted for sweep the funds and also add / remove tokens as well as per readMe we can relay on passed tranche data via sender.

Tranches data is used to validate Merkle proof, see validMerkleProof at claim function, so if there is anything wrong with passed data it won't verified and transaction would revert ( admin sets Merkle Root )

amankakar commented 5 months ago

The admin is only trusted for sweep the funds and also add / remove tokens as well As per contest readme. So we can't trust admin for right trance data and also the fraction denominator is used in smart contract. Either verify the last tranche vested fraction or return the fraction denominator as I suggested in my finding. Have you discussed it with the sponsor??

jkoppel commented 5 months ago

So I've been asked to comment.

It's pretty clear that the data in the Merkle tree needs to pass the same validations as would be done on-chain for the corresponding set function. Any Merkle root that contains improper tranche data, e.g..: tranches whose last tranche is not fractionDenominator, would be invalid input. Admin input validation errors are not valid issues.

Hash01011122 commented 5 months ago

I don't think admin input validation error argument is applicable for this issue. The report clearly highlights the logical flaw in tokensoft contract, which incorrectly calculates the last vesting of user. It would be better if @MxAxM can take clarification of this issue from the sponsors.

jkoppel commented 5 months ago

How wouldn't it be? This issue requires that the admin initialize the contract with a Merkle tree containing invalid tranche data.

The logic is perfectly correct, and is identical to that in TrancheVestingMerkle. It just assumes valid tranche data.

jkoppel commented 5 months ago

Also, this issue is not even escalated -- not sure why so much ink is being spilled on it.

amankakar commented 5 months ago

Also, this issue is not even escalated -- not sure why so much ink is being spilled on it.

So if some one is not able to escalate his issue , he can't ask the judge and add further clarification. I am not sure why you say it.

The best solution is to discuss it with the sponsor as @Hash01011122 he has said.