DMDcoin / diamond-contracts-claiming

claiming contracts
0 stars 2 forks source link

[M-02] Insufficient Timestamp Validation in Dilution Events #52

Open softstackio opened 2 months ago

softstackio commented 2 months ago

Description: The ClaimContract contains two significant issues related to the validation of dilution event timestamps:

  1. Lack of Minimum Time Gap: While the contract ensures that each dilution timestamp is greater than the previous one, it does not enforce a minimum time gap between dilution events. This could allow dilution events to occur in rapid succession, potentially disrupting the intended gradual dilution process.
  2. Missing Upper Bound Check: The contract does not implement an upper bound check for dilution timestamps. This oversight allows for the setting of extremely large timestamp values, which could lead to overflow issues or render dilution events practically impossible to occur.

Impact: These issues could have several negative consequences:

  1. Rapid Dilution: Without a minimum time gap, the contract owner could set dilution timestamps very close to each other, potentially diluting user balances more quickly than intended. This could undermine user trust and the fairness of the claiming process.
  2. Permanent Token Lock: Setting extremely high timestamp values could effectively lock tokens indefinitely, as the dilution events would never occur within a reasonable timeframe.
  3. Potential Overflow: Large timestamp values might cause overflow issues in other parts of the contract or in external systems interacting with it.

Proof of Concept: Consider the following scenario:

/ Current block.timestamp: 1000000000 (May 2001)
ClaimContract contract = new ClaimContract(
   beneficorAddress1,
   beneficorAddress2,
   "prefix",
   1000000001, // 1 second after deployment
   1000000002, // 2 seconds after deployment
   1000000003  // 3 seconds after deployment
);

In this example, all dilution events could occur within 3 seconds, which is likely not the intended behavior for a gradual dilution process. Alternatively:

// Current block.timestamp: 1000000000 (May 2001)
ClaimContract contract = new ClaimContract(
   beneficorAddress1,
   beneficorAddress2,
   "prefix",
11579208923731619542357098500868790785326998466564056403945758400791312963
9935, // max uint256 value
11579208923731619542357098500868790785326998466564056403945758400791312963
9935,
11579208923731619542357098500868790785326998466564056403945758400791312963
9935
);

This setup would effectively prevent any dilution from ever occurring, as the timestamps are set to the maximum possible value for uint256. Recommendation: To address these issues, consider implementing the following changes:

  1. Enforce Minimum Time Gaps: Add checks in the constructor to ensure a minimum time difference between dilution events:
uint256 constant MIN_DILUTION_GAP = 30 days;
constructor(...) {
   // Existing checks
   if (_dilute_s2_50_timestamp <= _dilute_s1_75_timestamp +
MIN_DILUTION_GAP)
       revert InsufficientDilutionGap();
   if (_dilute_s3_0_timestamp <= _dilute_s2_50_timestamp +
MIN_DILUTION_GAP)
       revert InsufficientDilutionGap();
// ...
}
  1. Add a reasonable upper limit for dilution timestamps:
    uint256 constant MAX_TIMESTAMP = 2524608000; // January 1, 2050
    constructor(...) {
    // Existing checks
    if (_dilute_s1_75_timestamp > MAX_TIMESTAMP) revert TimestampTooHigh();
    if (_dilute_s2_50_timestamp > MAX_TIMESTAMP) revert TimestampTooHigh();
    if (_dilute_s3_0_timestamp > MAX_TIMESTAMP) revert TimestampTooHigh();
    // ...
    }