CooldownManager : inconsistency in enforcing the minimum cooldownPeriod

[hats-bug-reporter[bot]]

Github username: @aktech297 Twitter username: kaka Submission hash (on-chain): 0x583fb6a91d863ebe8f4063b2ee4d373de719893d30576ed2bea94941d3481159 Severity: low

Description: Description\

The minimum cooldownPeriod should be greater than 1 min and less than 14 days. This can be seen in the setCoolDownPeriod.

Whenever deposit is made, the cooldown period is updated by calling the function _calculateCooldownPeriod

In above function,

Previous Cooldown Calculation: The function considers the remaining cooldown time from any previous deposits. Current Balance Check: Different logic paths are taken based on whether the current balance is solely from newly minted liquidity or a mix. Average Cooldown: An average cooldown is calculated for a smooth user experience, ensuring fairness when blending old and new liquidity. Minimum Cooldown: The cooldown is always at least 1 second to prevent issues with zero cooldown periods unless explicitly set by minted liquidity being zero.

lets see the last point, in the else block when calculating the cooldown,

CooldownManager.sol#L53-L64

    } else {
      // Calculate average cooldown based on the proportion of existing and new liquidity.
      uint256 balanceBeforeMint = _currentUserBalance - _mintedLiquidity;
      uint256 averageCooldown = (_mintedLiquidity *
        _defaultCooldownTime +
        balanceBeforeMint *
        prevCooldownRemaining) / _currentUserBalance;
      // Ensure the cooldown does not exceed the current cooldown setting and is at least 1 second.
      cooldown = averageCooldown > _defaultCooldownTime
        ? _defaultCooldownTime
        : MathUtils._max(averageCooldown, 1);
    }

The calculation updates the max(averageCooldown, 1) i.e 1 second is increased if the averageCooldown divide down to zero due to truncation.

But as shown in the configuration check, the minimum cooldown should be > 1 mins.

Impact\ It breaks one of the protocol invariant which decide the coold down period.

Attachments

1. Proof of Concept (PoC) File

code link where issue present

2. Revised Code File (Optional)

Check for 1 min in the else block.

(https://github.com/hats-finance/Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77/blob/aa47c9ff85bcc2bede62978c3895668b549da125/contracts/core/cooldown/CooldownManager.sol#L60-L63)

      // Ensure the cooldown does not exceed the current cooldown setting and is at least 1 second.
      cooldown = averageCooldown > _defaultCooldownTime
        ? _defaultCooldownTime
        : MathUtils._max(averageCooldown, 1 min); -->> @ audit corrected

[0xWeb3boy]

Even though it is incorrectly implemented I don't think it would impose any problem to the system, since averageCooldown is actually weighted average and will always be on the higher side i you submit smaller liquidity and would never be 1 second in that case.

      uint256 balanceBeforeMint = _currentUserBalance - _mintedLiquidity;
      uint256 averageCooldown = (_mintedLiquidity *  _defaultCooldownTime + balanceBeforeMint * prevCooldownRemaining) / _currentUserBalance;)

[aktech297]

Even though it is incorrectly implemented I don't think it would impose any problem to the system, since averageCooldown is actually weighted average and will always be on the higher side i you submit smaller liquidity and would never be 1 second in that case.

      uint256 balanceBeforeMint = _currentUserBalance - _mintedLiquidity;
      uint256 averageCooldown = (_mintedLiquidity *  _defaultCooldownTime + balanceBeforeMint * prevCooldownRemaining) / _currentUserBalance;)

its not always less than 1 seconds.. it would be less than 59 seconds. again it breaks the protocol assumption on the cooldown period.

[langnavina97]

The current implementation is correct. The cooldown period calculation considers the proportion of existing and new liquidity. It ensures there are no zero cooldown periods unless explicitly set by the protocol, with a 1-second minimum safeguard to prevent zero cooldown. There is no protocol requirement mandating a minimum cooldown period. Therefore, no changes are necessary.

@aktech297

[aktech297]

The current implementation is correct. The cooldown period calculation considers the proportion of existing and new liquidity. It ensures there are no zero cooldown periods unless explicitly set by the protocol, with a 1-second minimum safeguard to prevent zero cooldown. There is no protocol requirement mandating a minimum cooldown period. Therefore, no changes are necessary.

@aktech297

Hi @langnavina97

We saw the following code block where it restrict the minimum cool down to be greater than 1 min.

SystemSettings.sol#L46-L53

   */
  function setCoolDownPeriod(
    uint256 _newCooldownPeriod
  ) external onlyProtocolOwner {
    if (_newCooldownPeriod < 1 minutes || _newCooldownPeriod > 14 days) -------->>@@ checks here.
      revert ErrorLibrary.InvalidCooldownPeriod();
    cooldownPeriod = _newCooldownPeriod;
    emit CooldownPeriodUpdated(_newCooldownPeriod);
  }

So, we thought that 1 second is contradicting with the with min cool down.