Ironsidesec - `maxDelay` can be breached and causes DOS to most claimers

[sherlock-admin4]

Ironsidesec

high

maxDelay can be breached and causes DOS to most claimers

Summary

FairQueueInitializable.getFairDelayTime gives a notice on L58 below, that the delay time will be between [0, maxDelay]. But for over 50% of the users, it will be > max delay time due to the delay computing time depending on user's address and not accounting maxdelay modulo(%). All it has to do to solve is, do a modulo by maxDelay. so (getFairDelayTime() % maxDelay) and now it will be < maxDelay all the time and no DOS to the claimers.

Impact : DOS to half the users. Likelihood : whenever claim is called, so all the time.

Look at the way delay time is computed, a randomValue and distancePerSecond is computed when deployment and they are used on L78 and L82 below. This accounting doesn't guarantee that the delay time will be < max delaytime.

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/factory/FairQueueInitializable.sol#L56

2024-05-tokensoft-distributor-contracts-update\contracts\packages\hardhat\contracts\claim\factory\FairQueueInitializable.sol
38:     function _setPseudorandomValue(uint160 salt) internal {
39:         if (distancePerSecond == 0) {
41:             return;
42:         }
43:         require(salt > 0, "I demand more randomness");
44:         randomValue = uint160(uint256(blockhash(block.number - 1))) ^ salt;
45:     }

51:     function _setDelay(uint160 _maxDelayTime) internal {
52:         maxDelayTime = _maxDelayTime;
53:         distancePerSecond = _maxDelayTime > 0 ? type(uint160).max / _maxDelayTime : 0;
54:         emit SetDelay(_maxDelayTime);
55:     }

57:     /**
58: >>>  * @notice get a fixed delay for any address by drawing from a unform distribution over the interval [0, maxDelay]

70:      */
71:     function getFairDelayTime(address user) public view returns (uint256) {
72:         if (distancePerSecond == 0) {
73:             // there is no delay: all addresses may participate immediately
74:             return 0;
75:         }
78:         uint160 distance = uint160(user) ^ randomValue;
82:         return distance / distancePerSecond;
83:     }

Vulnerability Detail

DOS occurs in both TrancheVesting and ContinuousVesting.

1. In TrancheVesting, imagine a 3 tranch vesting 33% each for 3 months straight. And assuming max delay time is 5 days to combat gas wars. the delay time can be so high as it doesn't account to do (% maxdelay), and the vested fraction will return 0 due to high delay because the delayed time is  < first tranche's time, and claimable == 0 leads to 0 claimable amount. And as a user this is a DOS to me, most of the time. And I have to wait till my delayed time crosses each tranche's time. If the delayed time - current is < max delay, then it is intended and not a DOS. But delaying more than maxdelay is DOS.

Same issue onContinuousVesting, the delayed time will be < cliff most of the times even if the current time went > end, because the delayed time is so small with huge delay > maxdelay. Example: start on Jan 1, cliff on Feb 1, end on may 1 and max delay of 5 days. Even if the current time is on June 1, the delayed time iis made so small it goes to the previous year the maxdelay time of 5 days accounting is missing.

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/abstract/PerAddressTrancheVesting.sol#L41

2024-05-tokensoft-distributor-contracts-update\contracts\packages\hardhat\contracts\claim\abstract\PerAddressTrancheVesting.sol

28:   function getVestedFraction(
... SNIP ...
32:   ) public view override returns (uint256) {
33:     Tranche[] memory tranches = abi.decode(data, (Tranche[]));
34: 
35:  >> uint256 delay = getFairDelayTime(beneficiary);
36:     for (uint256 i = tranches.length; i != 0; ) {
37:       unchecked {
38:         --i;
39:       }
40: 
41:  >>>  if (time - delay > tranches[i].time) { 
42:         return tranches[i].vestedFraction;
43:       }
44:     }
46:     return 0;
47:   }

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/abstract/PerAddressContinuousVesting.sol#L29

2024-05-tokensoft-distributor-contracts-update\contracts\packages\hardhat\contracts\claim\abstract\PerAddressContinuousVesting.sol

20:     function getVestedFraction(
... SNIP ...
24:     ) public view override returns (uint256) {
25:     (uint256 start, uint256 cliff, uint256 end) = abi.decode(data, (uint256, uint256, uint256));
26: 
27:         uint256 delayedTime = time- getFairDelayTime(beneficiary); 
28:         // no tokens are vested
29:   >>>   if (delayedTime <= cliff) {
30:             return 0;
31:         }
32: 
33:         if (delayedTime >= end) {
34:             return fractionDenominator;
35:         }
36: 
37:         return (fractionDenominator * (delayedTime - start)) / (end - start);
38:     }

Impact

DOS to half the claimers of both TrancheVesting and ContinuousVesting so High impact and high likelihood.

Code Snippet

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/factory/FairQueueInitializable.sol#L56

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/abstract/PerAddressTrancheVesting.sol#L41

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/abstract/PerAddressContinuousVesting.sol#L29

Tool used

Manual Review

Recommendation

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/factory/FairQueueInitializable.sol#L79

 function getFairDelayTime(address user) public view returns (uint256) {
 if (distancePerSecond == 0) {
 // there is no delay: all addresses may participate immediately
 return 0;
 }

 // calculate a distance between the random value and the user's address using the XOR distance metric (c.f. Kademlia)
 uint160 distance = uint160(user) ^ randomValue;

 // return the delay (seconds)
-       return distance / distancePerSecond;
+       return (distance / distancePerSecond) % maxDelayTime;
 }

[IronsideSec]

@MxAxM, please share the rejection response.

[MxAxM]

@MxAxM, please share the rejection response.

Can you provide a PoC for this issue ?

[IronsideSec]

@MxAxM, please share the rejection response.

Can you provide a PoC for this issue ?

Tested a POC, it works as intended. the fair delay is between 0 and max delay only. Issue should be kept invalid.

Ran 1 test for test/foundry/DelayTest.t.sol:ContinuousVestingMerkleDistributorTest
[PASS] test_Delay() (gas: 22577)
Logs:
  3048
  3600
  405972677036361921723245786865634172126647928

Traces:
  [22577] ContinuousVestingMerkleDistributorTest::test_Delay()
    ├─ [8010] 0x815F194fa08ECeae9A97eb5714D2A95F51680Cc6::getFairDelayTime(0x36A35fB10d9d273da615f4b658829901326e0d00) [staticcall]
    │   ├─ [5338] ContinuousVestingMerkleDistributor::getFairDelayTime(0x36A35fB10d9d273da615f4b658829901326e0d00) [delegatecall]
    │   │   └─ ← [Return] 3048
    │   └─ ← [Return] 3048
    ├─ [0] console::log(3048) [staticcall]
    │   └─ ← [Stop] 
    ├─ [3684] 0x815F194fa08ECeae9A97eb5714D2A95F51680Cc6::maxDelayTime() [staticcall]
    │   ├─ [3518] ContinuousVestingMerkleDistributor::maxDelayTime() [delegatecall]
    │   │   └─ ← [Return] 3600
    │   └─ ← [Return] 3600
    ├─ [0] console::log(3600) [staticcall]
    │   └─ ← [Stop] 
    ├─ [1398] 0x815F194fa08ECeae9A97eb5714D2A95F51680Cc6::distancePerSecond() [staticcall]
    │   ├─ [1232] ContinuousVestingMerkleDistributor::distancePerSecond() [delegatecall]
    │   │   └─ ← [Return] 405972677036361921723245786865634172126647928 [4.059e44]
    │   └─ ← [Return] 405972677036361921723245786865634172126647928 [4.059e44]
    ├─ [0] console::log(405972677036361921723245786865634172126647928 [4.059e44]) [staticcall]
    │   └─ ← [Stop] 
    └─ ← [Return]