User Can Claim More Than totalAmount Due to Lack of Max Return Amount Check in _vested Function

[hats-bug-reporter[bot]]

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

Description: Description\ The _vested function in the Vesting contract has a potential issue where users can claim more than the total vested amount due to improper logic handling of initial unlock and no max return value check(refer POC)

Attack Scenario\ if the user deposits 500,000 tokens with initialUnlockAmount 50,000 then user can claim ~549000 tokens(i.e ~49k extra tokens) just before the end of duration

Attachments

1. Proof of Concept (PoC) File

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

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

contract TapiocaTest is Test{

//alex:
//totalAmout = 500,000;
//initial unlock = 10% i.e 50,000
//duration = 4 years

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

uint256 public  cliff = 0;

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

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

uint __initialUnlockTimeOffset = 0; //update in test_initial function

uint totalAmount = 500000;
uint initialUnlockAmount = 50000;

function _computeTimeFromAmount(uint256 _start, uint256 _totalAmount, uint256 _amount, uint256 _duration)
    public
    pure
    returns (uint256)
{
    return _start - (_start - ((_amount * _duration) / _totalAmount));
}

 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

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

function test_initial() public{

    console.log("start(5 june 2024) : ",start);
    console.log("duration(5 june 2028) : ",duration);
    console.log("totalAmount : ",totalAmount);
    console.log("initialUnlockAmount : ",initialUnlockAmount);

    uint  initialUnlockTimeOffset = _computeTimeFromAmount(start, totalAmount, initialUnlockAmount, duration);//unlocking 10% i,e 50,000 of 500,000
    console.log("initialUnlockTimeOffset : ", initialUnlockTimeOffset);
    __initialUnlockTimeOffset =  initialUnlockTimeOffset;

    vm.warp(start+4*364 days);//user will get extra 48630 tokens  this is due to adjustment in the _vested function  i.e ` _start = _start - __initialUnlockTimeOffset;` and no max return value check i.e totalAmount.due to adjustment in the `_start` the tokens will be fully vested before the duration now user can wait upto _duration-1 days and claim 548630 i.e more than totalAmount
    uint vestedAmount = _vested(totalAmount);
    console.log("vestedAmount just before the duration : ",vestedAmount);//here user just call the claim function just before the duration is completed and claim a huge 548630 amount tokens

    vm.warp(start+4*365 days);
     uint vestedAmount_afterDuration = _vested(totalAmount);
    console.log("vestedAmount after the duration : ",vestedAmount_afterDuration);

}

}

forge test --match-test test_initial -vvv

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

Ran 1 test for test/tap.t.sol:TapiocaTest [PASS] test_initial() (gas: 46261) Logs: start(5 june 2024) : 1717575083 duration(5 june 2028) : 126144000 totalAmount : 500000 initialUnlockAmount : 50000 initialUnlockTimeOffset : 12614400 vestedAmount just before the duration : 548630 vestedAmount after the duration : 500000

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

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

2. **Revised Code File (Optional)**
<!-- If possible, please provide a second file containing the revised code that offers a potential fix for the vulnerability. This file should include the following information:
- Comment with a clear explanation of the proposed fix.
- The revised code with your suggested changes.
- Any additional comments or explanations that clarify how the fix addresses the vulnerability. -->

working on it....

**Files:**
  - Vesting.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmfPzxRGGypsnB8PrMyaGaiSHHmMt29HWnPdXdZzQqeVQm)
  - tap.t.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmSZkcGG1oVKvXA8P3wXeCQGDNCKA4CGgsJERrfUctpoZr)

[maarcweiss]

Hi! Can you open a private thread on discord in hats channel to communicate about this issue? Ty

[maarcweiss]

Just to point out, we are checking the submission. We also have a 1 year cliff and then a 3 year linear unlock, which on the PoC it is set without cliff

[0xRektora]

This is wrong, the PoC removed one piece of code

     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

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

There is a missing - __initialUnlockTimeOffset. Ours

    function _vested(uint256 _totalAmount) internal view returns (uint256) {
        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
    }

Running the PoC again with ours.

 start(5 june 2024) :  1717575083
  duration(5 june 2028) :  126144000
  totalAmount :  500000
  initialUnlockAmount :  50000
  initialUnlockTimeOffset :  12614400
  vestedAmount just before the duration :  500000
  vestedAmount after the duration :  500000

[NishantKoyalwar]

hey @maarcweiss @0xRektora thank u for the response,i rechecked the issue (i mistakenly modified the function for the #36 Revised Code) but as i rechecked , there is still a issue present in this.

user tokens will be fully vested before the duration (4 years ) specifically 143 days early(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{

    //alex:
    //totalAmout = 500,000;
    //initial unlock = 10% i.e 50,000
    //duration = 4 years

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

    uint256 public  cliff = 0;

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

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

    uint __initialUnlockTimeOffset = 0; //update in test_initial function

    uint totalAmount = 500000;
    uint initialUnlockAmount = 50000;

    function _computeTimeFromAmount(uint256 _start, uint256 _totalAmount, uint256 _amount, uint256 _duration)
        public
        pure
        returns (uint256)
    {
        return _start - (_start - ((_amount * _duration) / _totalAmount));
    }

        function _vested(uint256 _totalAmount) internal view returns (uint256) {
        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 test_initial() public{

        console.log("start(5 june 2024) : ",start);
        console.log("duration(5 june 2028) : ",duration);
        console.log("totalAmount : ",totalAmount);
        console.log("initialUnlockAmount : ",initialUnlockAmount);

        uint  initialUnlockTimeOffset = _computeTimeFromAmount(start, totalAmount, initialUnlockAmount, duration);//unlocking 10% i,e 50,000 of 500,000
        console.log("initialUnlockTimeOffset : ", initialUnlockTimeOffset);
        __initialUnlockTimeOffset =  initialUnlockTimeOffset;

        vm.warp(start+3*364 days+222 days);//this issue is because in _vested function `_start = _start - __initialUnlockTimeOffset;` is applied ,this is pratically reducing the duration by __initialUnlockTimeOffset days.i.e no need to wait for 4 years for the tokens to be fully vested
        uint vestedAmount = _vested(totalAmount);
        console.log("vestedAmount before the duration is completed : ",vestedAmount);//here users tokens will be fully vested 143 days before the duration i.e 4 years

    }
}

[NishantKoyalwar]

any update?

[NishantKoyalwar]

below is the poc showing the issue with the example of [Early Supports](https://docs.tapioca.xyz/tapioca/token-economy/tap-distribution-and-issuance#:~:text=3.5%25%20(3%2C500%2C000%20TAP,0.22%20per%20TAP. )

Early Supporters

totalAmout = 3,500,000 TAP initial unlock = 210,000 TAP duration = 2 years

here early supports receive tokens before the completion 2 years

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

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

contract TapiocaTest is Test{

    //Early Supporters:
    //totalAmout = 3,500,000 TAP
    //initial unlock = 210,000 TAP
    //duration = 2 years

    uint256 public start = 1718286203;//current timestamp

    uint256 public  cliff = 0;

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

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

    uint __initialUnlockTimeOffset = 0; //update in test_initial function

    uint totalAmount = 3500000;// 3,500,000 TAP
    uint initialUnlockAmount = 210000;// 210,000 TAP

    function _computeTimeFromAmount(uint256 _start, uint256 _totalAmount, uint256 _amount, uint256 _duration)
        public
        pure
        returns (uint256)
    {
        return _start - (_start - ((_amount * _duration) / _totalAmount));
    }

        function _vested(uint256 _totalAmount) internal view returns (uint256) {
        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 test_initial() public{

        console.log("start : ",start);
        console.log("duration: ",duration);
        console.log("totalAmount : ",totalAmount);
        console.log("initialUnlockAmount : ",initialUnlockAmount);

        uint  initialUnlockTimeOffset = _computeTimeFromAmount(start, totalAmount, initialUnlockAmount, duration);
        console.log("initialUnlockTimeOffset : ", initialUnlockTimeOffset);
        __initialUnlockTimeOffset =  initialUnlockTimeOffset;

        vm.warp(start+687 days);//this issue is because in _vested function `_start = _start - __initialUnlockTimeOffset;` is applied ,this is pratically reducing the duration by __initialUnlockTimeOffset days.i.e no need to wait for 2 years for the tokens to be fully available
        uint vestedAmount = _vested(totalAmount);
        console.log("vestedAmount before the duration is completed : ",vestedAmount);//here users tokens will be fully claimable berfore the completion of the duration i.e 2 years

        //here early supportors will be able to claim the tokens before the 2 years of duration

    }
}

-> forge test --match-test test_initial -vvv