Protocol makes wrong assumption that could cause integration mistakes
Summary
Protocol makes wrong assumption that could cause integration mistakes
Vulnerability Detail
Upon claiming in tranches, users get a specific fraction of their total claimable amount based on the tranches specified in the merkle tree. However, protocol makes a wrong assumption that could lead to huge mistakes by the protocols integrating it.
/**
* @notice Get the vested fraction for a beneficiary at a given time.
* @dev Before the first tranche time, the vested fraction will be 0. At times between
* tranche_i and tranche_i+1, the vested fraction will be tranche_i+1's vested fraction.
* After the last tranche time, the vested fraction will be the fraction denominator.
*/
function getVestedFraction(address beneficiary, uint256 time, bytes memory data) public view override returns (uint256) {
Tranche[] memory tranches = abi.decode(data, (Tranche[]));
uint256 delay = getFairDelayTime(beneficiary);
for (uint256 i = tranches.length; i != 0;) {
unchecked {
--i;
}
if (time - delay > tranches[i].time) {
return tranches[i].vestedFraction;
}
}
return 0;
}
Take a look at their last comment in the function natspec:
After the last tranche time, the vested fraction will be the fraction denominator.
The vested fraction will not be the fraction denominator, it will be the vested fraction which in most cases will equal to fraction denominator but not always. The protocol assumes and also doesn't verify at all, that the given tranches' fractions will look something like this: [10, 50, 100] while a protocol might believe the proper way to do it is: [10, 40, 50]. One of them shows the whole current unlocked fraction while the other one shows the newly unlocked fraction for each tranche. Since it is never mentioned how exactly the tranche input is supposed to look like and with that misleading comment, such a scenario is very possible. This will make it so the user can only withdraw 50% of his total funds scheduled for claiming.
Furthermore, if there is no validation of the tranches data given by a user upon claiming (there actually isn't even right now), users can claim a vested fraction that is not 100 but 1000000 or any arbitrary amount allowing them to essentially drain the contract at any moment.
validMerkleProof(keccak256(abi.encodePacked(index, beneficiary, amount)), merkleProof) <@ No validation of tranches data given
Impact
Protocol makes wrong assumption that could cause integration mistakes
Validate the tranches data upon submitting the merkle root, upon claiming and have some validation in the getVestedFraction() function to make sure someone is not withdrawing more than the fraction denominator.
samuraii77
medium
Protocol makes wrong assumption that could cause integration mistakes
Summary
Protocol makes wrong assumption that could cause integration mistakes
Vulnerability Detail
Upon claiming in tranches, users get a specific fraction of their total claimable amount based on the tranches specified in the merkle tree. However, protocol makes a wrong assumption that could lead to huge mistakes by the protocols integrating it.
Take a look at their last comment in the function natspec:
After the last tranche time, the vested fraction will be the fraction denominator.The vested fraction will not be the fraction denominator, it will be the vested fraction which in most cases will equal to fraction denominator but not always. The protocol assumes and also doesn't verify at all, that the given tranches' fractions will look something like this:[10, 50, 100]while a protocol might believe the proper way to do it is:[10, 40, 50]. One of them shows the whole current unlocked fraction while the other one shows the newly unlocked fraction for each tranche. Since it is never mentioned how exactly the tranche input is supposed to look like and with that misleading comment, such a scenario is very possible. This will make it so the user can only withdraw 50% of his total funds scheduled for claiming.Furthermore, if there is no validation of the tranches data given by a user upon claiming (there actually isn't even right now), users can claim a vested fraction that is not 100 but 1000000 or any arbitrary amount allowing them to essentially drain the contract at any moment.
Impact
Protocol makes wrong assumption that could cause integration mistakes
Code Snippet
https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/factory/PerAddressTrancheVestingInitializable.sol#L31-L52
Tool used
Manual Review
Recommendation
Validate the tranches data upon submitting the merkle root, upon claiming and have some validation in the
getVestedFraction()function to make sure someone is not withdrawing more than the fraction denominator.