search

hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad

4 stars 4 forks source link

Delay in Full Vesting of user tokens Due to Incorrect Calculation #36

Open hats-bug-reporter[bot] opened 5 months ago

hats-bug-reporter[bot] commented 5 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x1f9e0bb4904bd2ab4940d601f25b084f4bd4426fd5f39c1ba24730a0b205ac0a Severity: high

Description: Description\ In the current implementation of the Vesting contract, there is an issue where the full vesting of tokens is delayed due to an incorrect calculation of the vesting duration. This occurs in the_vested function, where the _start time is incremented by the _cliff duration, and then the fully vested check is performed using_start + _duration. Since _start already includes the cliff period, this effectively extends the vesting duration by the cliff period, resulting in a delay in the full vesting of tokens(refer POC).

This could lead to tokens being vested over a longer period than expected, which could have significant implications for users expecting their tokens to be fully vested at the end of the specified vesting period.

Attack Scenario\ Let's say we have a vesting schedule with a one-year cliff and a total four-year vesting period. This means that no tokens are released for the first year (the cliff period), and then tokens are released gradually over the next three years. Start Time (_start): January 1, 2023 Cliff Period (_cliff): 1 year Vesting Duration (_duration): 4 years In the current implementation, the _start time is incremented by the _cliff duration in the _cliff > 0 check:

if (_cliff > 0) {
    _start = _start + _cliff; // Apply cliff offset
    if (block.timestamp < _start) return 0; // Cliff not reached
}

This means that _start now represents the time at which tokens start to vest, i.e., January 1, 2024. Then, the fully vested check is performed:

if (block.timestamp >= _start + _duration) return _totalAmount; // Fully vested

Here, _duration is the total vesting period, which is 4 years. So, the condition is checking if the current time is greater than or equal to the start of the vesting period plus the total vesting period. Incorrect Calculation: _start + _duration = January 1, 2024 + 4 years = January 1, 2028 However, since _start was already incremented by the cliff period, this effectively adds the cliff period twice, resulting in a total vesting period of 5 years instead of the intended 4 years. Intended Calculation: _start + _duration - _cliff = January 1, 2023 + 4 years = January 1, 2027 So, in our example, tokens would be fully vested after 5 years (January 1, 2028) instead of 4 years (January 1, 2027), which is not the intended behavior. The tokens should be fully vested after the start time plus the duration (4 years in this example), not after the start time plus the cliff period plus the duration (5 years in this example).

--->link_for_reference

Attachments

  1. Proof of Concept (PoC) File

    below is the test written in foundry

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;

import "forge-std/Test.sol";
//import "../src/Vesting.sol";

contract TapiocaTest is Test{

    uint256 public start = block.timestamp;

    uint256 public  cliff = 365 days;

    uint256 public duration = 4*365 days;

    address public owner = address(123);

    uint __initialUnlockTimeOffset = 0;

    function _vested(uint256 _totalAmount) public view returns (uint256) {//current implementation in the vesting.sol
        uint256 _cliff = cliff;
        uint256 _start = start;
        uint256 _duration = duration;

        if (_start == 0) return 0; // Not started

        if (_cliff > 0) {
            _start = _start + _cliff; // Apply cliff offset
            if (block.timestamp < _start) return 0; // Cliff not reached
        }

        if (block.timestamp >= _start + _duration) return _totalAmount; // Fully vested//@audit-here the fully vested will be unlocked after the start+cliff+duration not the start+duration

        _start = _start - __initialUnlockTimeOffset; // Offset initial unlock so it's claimable immediately
        return (_totalAmount * (block.timestamp - _start)) / _duration; // Partially vested
    }

    function Modified_vested(uint256 _totalAmount) public view returns (uint256) {//modified _vested function 
        uint256 _cliff = cliff;
        uint256 _start = start;
        uint256 _duration = duration;

        if (_start == 0) return 0; // Not started

        if (_cliff > 0) {
            _start = _start + _cliff; // Apply cliff offset
            if (block.timestamp < _start) return 0; // Cliff not reached
        }
        if(__initialUnlockTimeOffset >0){
             _start = _start - __initialUnlockTimeOffset; // Offset initial unlock so it's claimable immediately
             return (_totalAmount * (block.timestamp - _start)) / _duration; // Partially vested

        }

        _start = _start - _cliff;

        if (block.timestamp >= _start + _duration) return _totalAmount; // Fully vested

        //_start = _start - __initialUnlockTimeOffset; // Offset initial unlock so it's claimable immediately
        return (_totalAmount * (block.timestamp - _start)) / _duration; // Partially vested
    }

    function testVested() public {

        console.log("start : ",start);
        console.log("cliff : ",cliff);
        console.log("duration : ",duration);

        vm.warp(4*365 days);//duration = 4 years i.e tokens should be fully vested
        uint vestAmount = _vested(500000);//here as the duration is 4 years tokens should be fully vested but due to incorrect implementation users have to wait extra _cliff period for fully vest i.e users need to wait 5 years
        console.log("implemented vestAmount : ",vestAmount);
        uint new_vestAmount = Modified_vested(500000);//fully vested after the duration
        console.log("modified vestAmount : ",new_vestAmount);

    }
}

--->forge test --match-test testVested -vvv

[⠊] Compiling...
[⠆] Compiling 1 files with Solc 0.8.23
[⠰] Solc 0.8.23 finished in 1.17s
Compiler run successful!

Ran 1 test for test/tapioca_01.t.sol:TapiocaTest
[PASS] testVested() (gas: 19358)
Logs:
  start :  1
  cliff :  31536000
  duration :  126144000
  implemented vestAmount :  374999
  modified vestAmount :  499999

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 464.01µs (185.70µs CPU time)

Ran 1 test suite in 20.34ms (464.01µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

  1. Revised Code File (Optional)

    
function Modified_vested(uint256 _totalAmount) public view returns (uint256) {//modified _vested function 
    uint256 _cliff = cliff;
    uint256 _start = start;
    uint256 _duration = duration;

    if (_start == 0) return 0; // Not started

    if (_cliff > 0) {
        _start = _start + _cliff; // Apply cliff offset
        if (block.timestamp < _start) return 0; // Cliff not reached
    }
    if(__initialUnlockTimeOffset >0){
         _start = _start - __initialUnlockTimeOffset; // Offset initial unlock so it's claimable immediately
         return (_totalAmount * (block.timestamp - _start)) / _duration; // Partially vested

    }

    _start = _start - _cliff;

    if (block.timestamp >= _start + _duration) return _totalAmount; // Fully vested

    //_start = _start - __initialUnlockTimeOffset; // Offset initial unlock so it's claimable immediately
    return (_totalAmount * (block.timestamp - _start)) / _duration; // Partially vested
}

note:revised code is tested in the poc

**Files:**
  - tapioca_01.t.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmcfpWhqc39GGj7zGfyMjUbjvAEsicsGxzVA7MEtQ5xyKN)
  - Vesting.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmfPzxRGGypsnB8PrMyaGaiSHHmMt29HWnPdXdZzQqeVQm)
0xRektora commented 5 months ago

We appreciate the PoC, although this works, It seems that each one got a different approach to how cliff is supposed to act. In our case, the desired outcome is to literally wait for until the cliff ends, before starting to vest the tokens. The scenario you showcased is working as we intended it.

NishantKoyalwar commented 5 months ago

hey @0xRektora thank u for the response,i think there is slight misunderstanding in the issue,

In our case, the desired outcome is to literally wait for until the cliff ends, before starting to vest the tokens the current implementation works as intended i.e no tokens should be vested until the cliff ends ,the issue here is tokens will not be fully vested even if the duration is completed

let's examine this issue with example.

->alex registered for vesting, one-year cliff vesting period with a total four-year vesting period. alex receive no tokens during the first year (cliff), and after that, tokens vest monthly over the next 36 months. i.e the tokens should be fully vested after the 4 years

below is the detailed poc 

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;

import "forge-std/Test.sol";
//import "../src/Vesting.sol";

contract TapiocaTest is Test{

    //here alex is vesting his tokens for duration of 4 years with cliff of 1 year i.e here alex tokens should not vest until the cliff period is completed and he should get total tokens after the vesting period i.e _duration is completed

    //let's  understand with example

    //alex vest 500,000 tokens with cliff of 1 year and duration of 4 years i.e at the end of the duration i.e 4 years the alex tokens should be vested 

    //alex:
    //registed for vesting
    //start = 5 june 2024
    //cliff = 5 june 2025 //until this no should be vested
    //duration = 5 june 2028 //after this tokens should be fully vested 

    uint256 public start = 1717575083;//current timestamp i.e 5 june 2024 

    uint256 public  cliff = 365 days;//cliff of 1 year

    uint256 public duration = 4*365 days;//duration of 4 years

    address public owner = address(123);//owner of the contract

    uint __initialUnlockTimeOffset = 0;//initial unlock time offset of 0

    function _vested(uint256 _totalAmount) public view returns (uint256) {//current implementation in the vesting.sol
        uint256 _cliff = cliff;
        uint256 _start = start;
        uint256 _duration = duration;

        if (_start == 0) return 0; // Not started

        if (_cliff > 0) {
            _start = _start + _cliff; 
            if (block.timestamp < _start) return 0; //where the cliff period is not passed the tokens should not vest...this works as expected
        }

        if (block.timestamp >= _start + _duration) return _totalAmount; // Fully vested

        _start = _start - __initialUnlockTimeOffset; // Offset initial unlock so it's claimable immediately
        return (_totalAmount * (block.timestamp - _start)) / _duration; // Partially vested
    }

    function testVested() public {

        console.log("start(5 june 2024) : ",start);
        console.log("cliff(5 june 2025) : ",cliff);
        console.log("duration(5 june 2028) : ",duration);

        vm.warp(300 days);//checking for the cliff period i.e the tokens should not vest before the cliff is completed
        uint beforeCliff_vestAmount = _vested(500000);
        console.log("\n  vestAmount before cliff : ",beforeCliff_vestAmount);
        assertEq(beforeCliff_vestAmount, 0);//no amount of tokens should be vested before 5 june 2025 i.e cliff period

        vm.warp(start+4*365 days);//duration = 4 years(5 june 2028) i.e tokens should be fully vested
        uint vestAmount_afterDuration = _vested(500000);//here as the duration is 4 years tokens should be fully vested but due to incorrect implementation users have to wait an extra year
        console.log("5 june 2028 i.e tokens should be fully vested : ",vestAmount_afterDuration);
        vm.expectRevert();
        assertEq(vestAmount_afterDuration,500000);
        //as we can see the vestAmount_afterDuration is 375000, this is incorrect as the tokens should be fully vested i.e 500000

    }
}

->forge test --match-test testVested -vvv

tap

0xRektora commented 5 months ago

Hey @NishantKoyalwar , thanks for checking back on this. I still think there's a misunderstanding, correct me if I'm wrong.

On your test, what you essentially did was to vest for 3 years, not 4, because vesting only starts after the cliff.

Auditor's test:

Expected outcome of the protocol:

We revised your PoC to account for the above outcome, and the tokens were indeed fully vested by that time.

    function testVested() public {
        console.log("start(5 june 2024) : ", start);
        console.log("cliff(5 june 2025) : ", cliff);
        console.log("duration(5 june 2028) : ", duration);

        vm.warp(start + 300 days); //checking for the cliff period i.e the tokens should not vest before the cliff is completed
        uint256 beforeCliff_vestAmount = _vested(500000);
        console.log("\n  vestAmount before cliff : ", beforeCliff_vestAmount);
        assertEq(beforeCliff_vestAmount, 0); //no amount of tokens should be vested before 5 june 2025 i.e cliff period

        // @protocol - We need to wait for the cliff + the 4 years vestings.
        vm.warp(start + cliff + 4 * 365 days); //duration = 4 years(5 june 2028) i.e tokens should be fully vested
        uint256 vestAmount_afterDuration = _vested(500000); //here as the duration is 4 years tokens should be fully vested but due to incorrect implementation users have to wait an extra year
        // @protocol end 5 june 2029. start 2024, cliff ends 2025 + 4 years of vesting = 2029
        console.log("5 june 2029 i.e tokens should be fully vested : ", vestAmount_afterDuration);
        assertEq(vestAmount_afterDuration, 500000);
        //as we can see the vestAmount_afterDuration is 375000, this is incorrect as the tokens should be fully vested i.e 500000
    }
NishantKoyalwar commented 5 months ago

hey @0xRektora thank u for the response again,

i think there is a little misunderstanding in the cliff period and vest period,I appreciate the opportunity to clarify a few key points

1.

Key Terms Cliff Period: This is the initial period during which no tokens are vested. Only after this period will the tokens start to vest. Vesting Period: This is the total duration over which the tokens are gradually vested.

Vesting Plan

Cliff Period: 1 year
Total Vesting Duration: 4 years
Vesting Start Date: 5 June 2024
Cliff End Date: 5 June 2025 (No tokens are vested until this date)

How the Vesting Works

    Initial Year (Cliff Period): From 5 June 2024 to 5 June 2025, no tokens will be vested. This is the cliff period.
    Post-Cliff Period: After the cliff period ends on 5 June 2025, the tokens start vesting.
    Full Vesting: The tokens will continue to vest gradually over the remaining 3 years, reaching full vesting by 5 June 2028.

Correct Timeline

Vesting Start Date: 5 June 2024 Cliff End Date: 5 June 2025 (No tokens vested during this time) Vesting Period: After the cliff ends on 5 June 2025, tokens will vest gradually. Full Vesting Date: 5 June 2028 (4 years after the initial vesting start date, including the 1-year cliff period)

Example If 500000 tokens are to be vested over 4 years: No tokens are vested from 5 June 2024 to 5 June 2025. From 5 June 2025 to 5 June 2028, the tokens will vest gradually until fully vested. The misunderstanding was extending the total duration incorrectly. The total vesting duration is 4 years, including the 1-year cliff period.

  1. i have attached some relevent articles on the vesting logic below

investopedia

global shares

  1. then i thought checking the previous vesting logic implementation of Tapicao

    I dug deep into this and went through all the vesting logic implementations of Tapicao on github and found the following:

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/Vesting.sol#L167C5-L173C6

function _vested(uint256 _total) private view returns (uint256) {
        if (start == 0) return 0;
        uint256 total = _total;
        if (block.timestamp < start + cliff) return 0;
        if (block.timestamp >= start + duration) return total;
        return (total * (block.timestamp - start)) / duration;
    }

this works correctly i,e tokens will be fully vested by 5 june 2028 _start : 5 june 2024 _cliff : 1 year(5 june 2025) _duration : 4 years(5 june 2028)

no tokens will be vested until the cliff end(i.e 5 june 2025) and then tokens will be fully vested by 5 june 2028(i have attached poc at the end)

4.i considered verifying this logic with0xweissduring the submission

0xweiss

poc of the previous implementation mentioned in 3.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;

import "forge-std/Test.sol";
//import "../src/Vesting.sol";

contract TapiocaTest is Test{

    //here alex is vesting his tokens for duration of 4 years with cliff of 1 year i.e here alex tokens should not vest until the cliff period is completed and he should get total tokens after the vesting period i.e _duration is completed

    //let's  understand with example

    //alex vest 500,000 tokens with cliff of 1 year and duration of 4 years i.e at the end of the duration i.e 4 years the alex tokens should be vested 

    //alex:
    //registed for vesting
    //start = 5 june 2024
    //cliff = 5 june 2025 //until this no should be vested
    //duration = 5 june 2028 //after this tokens should be fully vested 

    uint256 public start = 1717575083;//current timestamp i.e 5 june 2024 

    uint256 public  cliff = 365 days;//cliff of 1 year

    uint256 public duration = 4*365 days;//duration of 4 years

    address public owner = address(123);//owner of the contract

    uint __initialUnlockTimeOffset = 0;//initial unlock time offset of 0

    function _vested(uint256 _total) public view returns (uint256) {
        if (start == 0) return 0;
        uint256 total = _total;
        if (block.timestamp < start + cliff) return 0;
        if (block.timestamp >= start + duration) return total;
        return (total * (block.timestamp - start)) / duration;
    }

    function testVested() public {

        console.log("start(5 june 2024) : ",start);
        console.log("cliff(5 june 2025) : ",cliff);
        console.log("duration(5 june 2028) : ",duration);

        vm.warp(300 days);//checking for the cliff period i.e the tokens should not vest before the cliff is completed
        uint beforeCliff_vestAmount = _vested(500000);
        console.log("\n  vestAmount before cliff : ",beforeCliff_vestAmount);
        assertEq(beforeCliff_vestAmount, 0);//no amount of tokens should be vested before 5 june 2025 i.e cliff period

        vm.warp(start+4*365 days);//duration = 4 years(5 june 2028) i.e tokens should be fully vested
        uint vestAmount_afterDuration = _vested(500000);//here as the duration is 4 years tokens should be fully vested but due to incorrect implementation users have to wait an extra year
        console.log("5 june 2028 i.e tokens should be fully vested : ",vestAmount_afterDuration);

        assertEq(vestAmount_afterDuration,500000);
        //as we can see the vestAmount_afterDuration is 500000, 

    }
}
NishantKoyalwar commented 5 months ago

@0xRektora?

0xRektora commented 5 months ago

The answer we gave is still holding. Judging stays invalid.

NishantKoyalwar commented 5 months ago

hey @0xRektora , i think there is misunderstanding . i have verified with @maarcweiss that there will be no tokens vestied until the _cliff period ends and the tokens will be vested for next 3 years i.e at the end of 4 years tokens should be fully vested. 0xweiss

0xweiss_01

NishantKoyalwar commented 5 months ago

@0xRektora @maarcweiss

i have also attached the tapicoa's previous _Vested function implementation ,

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/Vesting.sol#L167C5-L173C6

this works correctly i,e tokens will be fully vested by 5 june 2028 _start : 5 june 2024 _cliff : 1 year(5 june 2025)....no tokens will be vested until the 5 june 2025 _duration : 4 years(5 june 2028)...tokens will be fully vested at the end of 5 june 2028 i.e (1 year cliff + 3 years vesting)

note:the issue in the current implementation is the tokens will not be fully vested by end of 5 june 2028 i.e 4 years. rather users has to wait 5 years for tokens to be fully vested i.e more than the duration

NishantKoyalwar commented 5 months ago

Hey @0xRektora , I think there a misunderstanding between us on how vesting in general works. I have also reached out to my friends to validate if something is wrong on my end but everyone agreed.

As you have mentioned earlier in out conversation Expected outcome of the protocol: (https://github.com/hats-finance/Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad/issues/36#issuecomment-2149909412) , I get that from your POV total wait time for all tokens is cliff period + vesting time but this is completly opposite of what the industry norm is and counters the defination of vesting in general . Let me put my point through a simple example

Example

Total Tokens expected after full vesting : 48 Vesting Schedule/Time: 4 years Cliff Period: 1 year

What you believe/implemented in current contract

If i am not wrong this is how you expect the vesting happens

What the industry Norm is / I have shared

Coincident enough when i mean Industry Norm it is the same logic that you have implemented in previous version of tapioca https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/Vesting.sol#L167C5-L173C6

Verdict / Explaination

As you can see the issue / incorrect calculation is due to different definations of vesting that you have implemented vs what the industry Norm(including what previous version of tapioca) is . Vesting etc are common jargons in business industry and i was also assigned option grants by my employer when i first joined in exactly same way i have described earlier

I reiterate according to industry / norm Cliff period is part of Total Vesting Time and is not a external entity

Although this might be not necessary here is one of the reference for industry norm (https://carta.com/learn/equity/stock-options/vesting/).

I hope i have put my point forward in clear way. Let me know if you need any other external references or something else i would be happy to help.

P.S if you counter it would be very helpful if you explain it with same example

tapiocadao commented 5 months ago

@NishantKoyalwar Because it appears to me from reading prior correspondence we're having a level of talking past one another, could you please keep your response short to like one sentence without the commentary and links so we can ensure we're speaking to the root of the discussion- can you tell me how you believe this implementation differs from the tokens being completely locked for one year, than vesting being started at the end of the one year lock to then being linearly unlocked over the next three years?

If you believe the implementation is correct based on just that statement (NOT definitionally), and instead definitionally speaking believe the implementation is incorrect for a "one year lock and a three year vest", what are you saying a lock and vesting are? Is it that the tokens should be vesting during the lock? Again- trying to keep this short and precise so we can more effectively communicate, thank you

NishantKoyalwar commented 5 months ago

To put it simple for 4 years of vesting, in the current implementation user has to wait 5 years to claim total tokens. In my opinion it should be 4 years

tapiocadao commented 5 months ago

Again we're kind of missing each other, the example I gave was a one year lock, three year vest, which would be four years combined. You're talking about the example you gave, so we're talking past one another again. Can you answer the question I posed in the previous comment? Thanks

NishantKoyalwar commented 5 months ago

The incorrect implementation lies in the code if (block.timestamp >= _start + _duration) return _totalAmount; because _start is modified by adding the cliff period during the cliff check (_start = _start + _cliff). This results in tokens vesting after the 1-year cliff (as expected)plus an additional 4 years(‘start+1 year + 4 years’), totaling 5 years, which is incorrect. The tokens should vest over 3 years after a 1-year lock or cliff, not 5 years.

-> If it's still unclear, could we discuss this on a call (Discord)? I believe a call would help clarify and verify the issue with a proof of concept. Thank you!

tapiocadao commented 5 months ago

Hi, I'm really not trying to be harsh just effective for each of our valuable time- again we're not communicating here, you're answering questions I'm not asking and you're not answering the question I actually am asking, we are 4 days from going mainnet and we've spent close to a week already on this- I do not have even more time to spend on this, I'm simply asking you to answer one question and I'm not sure where the communication break down is coming from as this is a very direct question. I will paste the question again:

**Lets say we want to have a one year lock, than a three year vesting period. Would our code implementation enforce the tokens being locked completely for one year, vesting starting at the end of the lock-up period, and vest over 3 years linearly?

If our code implementation WILL follow that, but you believe the code doesn't fit the definition of cliff vesting / vesting, and the tokens should be vesting quicker than above, tell me that and explain.**

NishantKoyalwar commented 5 months ago

Thank you for the response. I'll keep this straight and to the point:

one year lock, than a three year vesting period

  1. Would the current code implementation enforce the tokens being locked completely for one year?

    • Yes, tokens will not be vested and will be locked for one year (cliff period).

  2. Does the vesting start at the end of the lock-up period?

    • Yes.

  3. Does it vest over three years linearly?

    • No. The current code implementation takes four years to fully vest tokens, excluding the one-year lock period.('bug')

Current Implementation Example:

Alex:

Correct Implementation Example:

Alex:

I hope this clarifies the issue. Please let me know if there are any further questions.

tapiocadao commented 5 months ago

Hey awesome, thank you for working with me on that I really appreciate it. This gets to the root of the discussion and what I thought maybe the issue was.

So I'm not sure if you noticed, the only use of lock-up within Tapioca vesting is here:

https://docs.tapioca.xyz/tapioca/token-economy/tap-distribution-and-issuance

15% (15,000,000 TAP) is allocated to the Tapioca Foundation. These tokens will be locked for 12 months starting at Genesis, then linearly unlocked per transaction block over the next 36 months thereafter. This lock-up period followed by a protracted vesting period ensures core contributors remain fully committed to the Tapioca ecosystem & community long-term.

Notice it says "locked" and not a cliff. This is because, while the definition of a cliff varies to many- some it means lockup, other it means unavailable but vesting- within Tapioca it's very rigidly a lock, not a cliff. While the code states cliff, the desired outcome is what is occurring which was also notated in the documentation.

NishantKoyalwar commented 5 months ago

Due to the incorrect implementation, the 15% (15,000,000 TAP) allocated to the Tapioca Foundation will be locked for 12 months starting at Genesis, and then linearly unlocked per transaction block over the next 48 months instead of the intended 36 months.

Clarified Understanding:

-After the lock-up period, the tokens should be vested linearly over the next 36 months.

Current Implementation Issue:

-The current implementation incorrectly extends the vesting period to 48 months instead of the intended 36 months.

Does this clarify the issue for you?

tapiocadao commented 5 months ago

So you are saying, that with the current implementation, if the contract was set to have a one year lock and after the lock period then have a three year linear unlock period, that after the one year lock duration, the tokens will then unlock over a course of 4 years (48 months) instead of the intended 3 year (36 months) unlock period?

If no, are you instead saying that definitionally, the one year lock should be included in the three year total unlock duration? So a one year cliff + three year vest, in your opinion in otherwords is a 3 year total vesting duration, not four.

Again, lets keep this precise for time efficiency. One of these statements has to be true for your submission, so please just so we can move expeditiously, tell me which one we're debating. Thanks

NishantKoyalwar commented 5 months ago

So you are saying, that with the current implementation, if the contract was set to have a one year lock and after the lock period then have a three year linear unlock period, that after the one year lock duration, the tokens will then unlock over a course of 4 years (48 months) instead of the intended 3 year (36 months) unlock period?

-->Yes!!,that's right

tapiocadao commented 5 months ago

Can you post a POC showing that, if you set the cliff (lock) to one year, and to then unlock over the next three years after, that it will unlock over four years after the one year lock instead of 3 years?

NishantKoyalwar commented 5 months ago

Sure, I am currently traveling. I will post the POC as soon as I get to my PC.

tapiocadao commented 5 months ago

Also, please refrain from setting the vesting to four years in your POC, please set the cliff to one year, and vesting to three years as we have discussed to show that it will instead incorrectly have a one year lock, but than unlock tokens over four years instead of the 3 years set, thank you.

the correct desired implementation is for a one year lock & 3 year vest is (100 tokens):

month 0 to month 12: no tokens released month 12-24: 33.3 tokens month 24-36: 33.3 tokens month 36-48: 33.3 tokens

for it to be incorrect and this be a valid issue: month 0 to month 12: a single token is released when it should not be takes longer than month 48 to receive all tokens takes less than 48 months to receive all tokens

NishantKoyalwar commented 5 months ago

below is the POC,showing that if you set the cliff (lock) to one year, and to then unlock over the next three years after, that it will unlock over four years after the one year lock instead of 3 years

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;

import "forge-std/Test.sol";
//import "../src/Vesting.sol";

contract TapiocaTest is Test{

    uint256 public start = 1;//start

    uint256 public  cliff = 365 days;//cliff of 1 year

    uint256 public duration = 4*365 days;//duration of 4 years(1 year cliff + 3 years vesting)

    uint __initialUnlockTimeOffset = 0;//initial unlock time offset of 0

    uint totalAmount = 100;

    function _vested(uint256 _totalAmount) public view returns (uint256) {//current implementation
        uint256 _cliff = cliff;
        uint256 _start = start;
        uint256 _duration = duration;

        if (_start == 0) return 0; // Not started

        if (_cliff > 0) {
            _start = _start + _cliff; // Apply cliff offset
            if (block.timestamp < _start) return 0; // Cliff not reached
        }

        if (block.timestamp >= _start - __initialUnlockTimeOffset + _duration) return _totalAmount; // Fully vested

        _start = _start - __initialUnlockTimeOffset; // Offset initial unlock so it's claimable immediately
        return (_totalAmount * (block.timestamp - _start)) / _duration; // Partially vested
    }

    function testVested_01() public {

        vm.warp(365 days);//lock period 
        uint first_year = _vested(totalAmount);
        console.log("month 0 to month 12: ",first_year);

        vm.warp(2*365 days);//1st year of vesting
        uint second_year = _vested(totalAmount);
        console.log("month 12 to month 24: ",second_year);

        vm.warp(3*365 days);//2nd year of vesting 
        uint third_year = _vested(totalAmount);
        console.log("month 24 to month 36: ",third_year);

        vm.warp(4*365 days);//3rd year of vesting..here at the end of 48 months tokens should be fully vested.
        uint fourth_year = _vested(totalAmount);
        console.log("month 36 to month 48: ",fourth_year);

        vm.warp(5*365 days);//delay
        uint fifth_year = _vested(totalAmount);
        console.log("month 48 to month 60: ",fifth_year);

    }
}

forge test --match-test testVested_01 -vvv

final_01

from this we can clearly see that, current implementation takes longer than month 48 to receive all tokens

tapiocadao commented 5 months ago

uint256 public duration = **4***365 days;//duration of 4 years(1 year cliff + 3 years vesting)

You set the cliff to one year, and the vesting to four years, as I knew you would do and that I explicitly told you not to do, and then are showing me it vests to four years, as you set it to vest to, and is the desired outcome.

100% invalid. I don't think you're doing this purposefully, but this should be a learning lesson for you to listen, read, and comprehend more, or you waste both your own time and the clients.

NishantKoyalwar commented 5 months ago

Hey,then what should be the duration according to u??I think it should be lock period + vesting period I.e (1+3=4 years).

NishantKoyalwar commented 5 months ago

like if you replace the _vested function in the poc with the previous _vested function implementation i.e

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/Vesting.sol#L167C4-L173C6

function _vested(uint256 _total) private view returns (uint256) {
        if (start == 0) return 0;
        uint256 total = _total;
        if (block.timestamp < start + cliff) return 0;
        if (block.timestamp >= start + duration) return total;
        return (total * (block.timestamp - start)) / duration;
    }

u get the expected results i.e user will receive all tokens at the end of 48 months

final_02

*if u still think this issue is invalid just compare the function tests/results with the previous implemented function(https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/Vesting.sol#L167C4-L173C6 ) attached above....because i am still very confident about this issue

tapiocadao commented 5 months ago

If you set the cliff to one year and the vesting to three years, it works as intended. Your problem is how we've implemented a cliff + vesting as far as implementation against the definition, and as I've shown you on the docs, it's not supposed to be a cliff it's an actual lockup period or a delay to start vesting, and regardless of what you think the definition is, it's implemented as intended. I'm not trying to be rude with you, I don't think you're being purposefully obtuse, but you're not grasping what myself and Rektora have been saying to you, and that has caused gigantic time sinks that were avoidable. Just trying to explain this to you for next time. The issue is 100% invalid though, and we're going to have to close this out.

NishantKoyalwar commented 5 months ago

ohh now i get it, duration should be equal to vesting period(3 years) ,not the vesting period+lock period(4 years)

NishantKoyalwar commented 5 months ago

hey @tapiocadao can u please check the latest comment on the #39