hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad

4 stars 4 forks source link

Initial Unlock Amount Not Immediately Claimable #37

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\ The Vesting contract contains a contradiction regarding the immediate claimability of the initial unlock amount. The comment in the init function suggests that the initial unlock amount should be claimable immediately. However, the implementation in the _vested function does not allow this if a cliff period is set and has not yet passed. Details In the init function, the __initialUnlockTimeOffset is calculated to allow the initial unlock amount to be claimed immediately:

if (_initialUnlock > 0) {
    uint256 initialUnlockAmount = (_seededAmount * _initialUnlock) / 10_000;
    __initialUnlockTimeOffset =
        _computeTimeFromAmount(block.timestamp, _seededAmount, initialUnlockAmount, duration);
}

However, in the _vested function, the start time is adjusted for the __initialUnlockTimeOffset after the cliff check:

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

This means that the initial unlock amount cannot be claimed immediately if the cliff period has not yet passed, regardless of the __initialUnlockTimeOffset or anything.

Attack Scenario\ Protocol Insolvency

Expected Behavior

Attachments

  1. Proof of Concept (PoC) File

    If the intention is to allow the initial unlock amount to be claimed immediately, regardless of the cliff period, then the _vested function should be modified to adjust the start time for the initial unlock before the cliff check.

https://github.com/Tapioca-DAO/tap-token/blob/baddafc15616a5674e73c2f5a920b92bf9b21888/contracts/tokens/Vesting.sol#L259C4-L275C6

  1. Revised Code File (Optional)
    • Suggested Fix

Update the _vested function to adjust the start time for the initial unlock before the cliff check:

Files:

0xRektora commented 5 months ago

While this can happen given the current parameters, in reality if a cliff exists, no __initialUnlockTimeOffset will be used, and vice versa. A check could be added in the init() if needed.

NishantKoyalwar commented 5 months ago

@0xRektora ??

NishantKoyalwar commented 5 months ago

hey @0xRektora ,i believe this issue is valid because -if the _cliff is set in the constructor and the init function is called with _initialUnlock,as there is no check ,the user has to wait 1year(_cliff period) i.e users cannot claim the initial unlock amount.

maarcweiss commented 5 months ago

this is actually not valid instead of a low as we will not have both at the same time cliff and __initialUnlockTimeOffset