Off-by-one timestamp error

[hats-bug-reporter[bot]]

Github username: @Jelev123 Twitter username: zhulien_zhelev Submission hash (on-chain): 0xedb30423344bcf94d7b876750d3126dd178d46dcea799a6ab0e81783ebd771f7 Severity: medium

Description: Description\ In Solidity, using >= or <= to compare against block.timestamp (alias now) may introduce off-by-one errors due to the fact that block.timestamp is only updated once per block and its value remains constant throughout the block's execution. If an operation happens at the exact second when block.timestamp changes, it could result in unexpected behavior. To avoid this, it’s safer to use strict inequality operators (> or <). For instance, if a condition should only be met after a certain time, use block.timestamp > time rather than block.timestamp >= time. This way, potential off-by-one errors due to the exact timing of block mining are mitigated, leading to safer, more predictable contract behavior.

The _checkCoolDownPeriod function in the Solidity contract is responsible for verifying whether a cooldown period has elapsed for a given user. The function calculates the remaining cooldown time based on the user's last deposit time and the specified cooldown duration. However, there is an off-by-one error in the comparison logic used to determine the remaining cooldown time. Specifically, the function uses a less-than-or-equal-to (<=) operator, which can lead to the cooldown period ending one block too early or one block too late, causing incorrect behavior.

Attack Scenario

• Cooldown Ends Early:

Suppose the cooldown period is meant to prevent withdrawals until block 100. Due to the off-by-one error, a user might be able to withdraw at block 100 instead of block 101. This can allow an attacker to bypass the intended security constraints of the cooldown period, potentially leading to premature withdrawals or other restricted actions.

• Cooldown Ends Late:

Suppose the cooldown period is meant to allow withdrawals starting from block 100. Due to the off-by-one error, the function might prevent withdrawals until block 101. This can cause frustration among users who are expecting to perform actions at the intended time, leading to a poor user experience.

Attachments

1. Proof of Concept (PoC) File https://github.com/Velvet-Capital/velvet-core/blob/849629b1aacf32d84634d8c4ef1378527bce3bb3/contracts/core/cooldown/CooldownManager.sol#L75

Recommendation

use

uint256 remainingCoolDown = userCoolDownPeriod < block.timestamp
      ? 0
      : userCoolDownPeriod - block.timestamp;

[langnavina97]

Thank you for your input, but this submitted issue is invalid.

It's not relevant if the cooldown period ends one block earlier or later. @Jelev123

[Jelev123]

Thank you for your input, but this submitted issue is invalid.

It's not relevant if the cooldown period ends one block earlier or later. @Jelev123

Thank you for your response. I understand that the perceived relevance of a one-block difference might seem minor at first glance, but I would like to clarify why addressing this off-by-one error is important for the contract's integrity and user experience.

In the current implementation, the comparison logic for the cooldown period uses a less-than-or-equal-to (<=) operator. This can lead to the cooldown period ending one block earlier or later than intended due to the nature of how block.timestamp is updated once per block.

• Premature Cooldown End (Potential Security Risk):

Scenario: Suppose the cooldown period is intended to prevent actions until block 100. Issue: Due to the off-by-one error, a user might be able to perform the action at block 100 instead of block 101. Impact: This can allow an attacker to bypass the intended security constraints, leading to premature withdrawals or other restricted actions. Even though it's a one-block difference, it could be exploited in certain situations, especially in high-value or time-sensitive contracts.

• Delayed Cooldown End (User Experience Issue):

Scenario: Suppose the cooldown period is intended to allow actions starting from block 100. Issue: The current implementation might prevent actions until block 101. Impact: Users expecting to perform actions at the intended time might face unnecessary delays, leading to a poor user experience. For applications requiring precise timing, such discrepancies can be significant.

@langnavina97