Insufficient validation in timelock calculations

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/main/src/core/absorber.cairo#L446

Vulnerability details

The code calculates the timelock for a request based on the current timestamp and a multiplier if a previous request has not cooled down:

let current_timestamp: u64 = get_block_timestamp();
let mut timelock: u64 = REQUEST_BASE_TIMELOCK;
if request.timestamp + REQUEST_COOLDOWN > current_timestamp {
    timelock = request.timelock * REQUEST_TIMELOCK_MULTIPLIER;
}
let capped_timelock: u64 = min(timelock, REQUEST_MAX_TIMELOCK);

The code also determines the current epoch based on the amount of yin provided by the caller:

let (new_provision_shares, issued_shares) = self.convert_to_shares(amount, false);
...
let current_epoch: u32 = self.current_epoch.read();

The code correctly calculates the timelock for a request and determines the current epoch. However, it does not explicitly validate that these calculations do not result in unexpected values. For example, without validation, the calculated timelock could exceed the maximum allowed timelock due to an excessively large REQUEST_TIMELOCK_MULTIPLIER or REQUEST_BASE_TIMELOCK, leading to unexpected delays in request processing. Similarly, the current epoch could be incorrectly determined if the convert_to_shares function returns an unexpected value.

Impact

Unexpected values in time-related calculations can have various negative impacts on the contract's functionality and security, including:

• Excessive timelocks could delay request processing, affecting the contract's usability and efficiency.
• Incorrect epoch determination could disrupt the contract's economic mechanisms, affecting rewards distribution, asset absorption, and more.

Mitigation

The contract should include validation checks to ensure that the calculated timelock and the determined epoch are within logical and expected bounds. Here's an example of how the code could be modified to include such checks:

let current_timestamp: u64 = get_block_timestamp();
let mut timelock: u64 = REQUEST_BASE_TIMELOCK;
if request.timestamp + REQUEST_COOLDOWN > current_timestamp {
    timelock = request.timelock * REQUEST_TIMELOCK_MULTIPLIER;
    assert(timelock <= REQUEST_MAX_TIMELOCK, 'ABS: Timelock exceeds maximum allowed value');
}
let capped_timelock: u64 = min(timelock, REQUEST_MAX_TIMELOCK);
...
let (new_provision_shares, issued_shares) = self.convert_to_shares(amount, false);
assert(new_provision_shares >= 0 && issued_shares >= 0, 'ABS: Invalid shares calculation');
let current_epoch: u32 = self.current_epoch.read();
assert(current_epoch >= FIRST_EPOCH, 'ABS: Invalid epoch determination');

In this modified code, assertions are added to validate the calculated timelock and the determined epoch, ensuring that they do not exceed the maximum allowed values or fall below the minimum expected values.

Assessed type

Invalid Validation

[c4-pre-sort]

bytes032 marked the issue as duplicate of #25

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid