6.1 Unbounded loop leading to index out of bound error prevents claiming of tokens
The simulate_amount_claimable(...) calls simulate_get_expanded_actual_claim_timestamps_and_bips(...) which returns expanded_claim_timestamps_array that contains all the unlock timestamps. It then loops through expanded_claim_timestamps_array to set the start and end indexes. It breaks when the timestamp at the current index is greater than or equal to the current timestamp, and this index will be the end index. The issue is that if the current timestamp is greater than the last timestamp at expanded_claim_timestamps_array, then it will not stop at the last index but will access the element beyond that which would lead to an index out-of-bound error, preventing users from claiming their tokens.
fn simulate_amount_claimable(
self: @ContractState, actual_start_timestamp_absolute: u64,
preset_linear_end_timestamp_relative: u64,
///.............................
/// ........ Existing code ......
///.............................
/// Expand linear timestamps so each unlock timestamp is explicitly represented
let (expanded_claim_timestamps_array, expanded_claim_bips_array) = self
.simulate_get_expanded_actual_claim_timestamps_and_bips(
preset_linear_start_timestamps_relative,
preset_linear_end_timestamp_relative,
actual_start_timestamp_absolute,
preset_num_of_unlocks_for_each_linear,
preset_linear_bips,
);
/// Validate if round ID timestamps are within an acceptable time range
let mut expanded_claim_timestamp_start_index = Bounded::MAX;
let mut expanded_claim_timestamp_end_index =
0; /// The most recent index that's not in the future, used as an endpoint for claim calculation
/// We can perform calculation in this loop first, then validate round
/// timestamps after. We can save one loop.
loop {
// @audit The expanded_claim_timestamp_end_index can be greater than the last index of the array, leading to index out-of-bound error
/// `expanded_claim_timestamp_end_index` is used as `i` here
let current_iteration_timestamp = *expanded_claim_timestamps_array
.at(expanded_claim_timestamp_end_index);
if expanded_claim_timestamp_start_index == Bounded::MAX
&& current_iteration_timestamp > actuals_chainlink_last_claimed_timestamp {
expanded_claim_timestamp_start_index = expanded_claim_timestamp_end_index;
}
if current_iteration_timestamp >= claim_timestamp_absolute {
break;
}
expanded_claim_timestamp_end_index += 1;
};
} else {
}
}
Recommendation
Consider breaking if all the elements are iterated.
Update from Nethermind Security
There's an issue with the fix that allows users to claim an extra unlock timestamp. For example, if expanded_claim_timestamps_array = [100, 200, 300] and claim_timestamp_absolute = 201, and since there's a check to break if current_iteration_timestamp >= claim_timestamp_absolute, the loop will break when current_iteration_timestamp is 300. As a result, expanded_claim_timestamp_end_index will be 2 (the last index), allowing the user to claim an extra unlock timestamp.
6.2 The amount_skipped is not accounted for when chainlink price feed is set
The amount_skipped is used while creating an Actual to allow the owner to set the amount claimed by the recipient. This will be accounted for when recipients claim the tokens by ensuring they will not get any tokens until amount_skipped is covered and only get the amount beyond it. This might be useful when the recipient has already received this amount from external sources, and the owner wants to account for this amount.
Thus, to account for this, when creating an Actual, the amount_claimed is set to amount_skipped. In the case when Chainlink price feed is disabled while claiming tokens, recipients get the token amount equal to the difference between token_amount_claimable_from_start and amount_claimed. However, when Chainlink price feed is enabled, the calculation for the amount of tokens the recipient would receive does not account for amount_skipped.
Recommendation
Consider modifying the calculation for the amount of tokens the recipient would receive in the case when Chainlink price feed is enabled to also account for amount_skipped.
Resolution
The client disabled amount_skipped when using Chainlink in commit 035c704
6.3 Chainlink round data manipulation in claim allows user to potentially claim more tokens than intended
In the TTUnlocker contract’s branch for schedules using Chainlink (in simulate_amount_claimable), the calculation of the claimable token amount depends on an array of round IDs supplied by the caller (via the parameter extra_chainlink_round_ids). Because these round IDs are used directly without independent validation, a user can try to supply “favorable” round IDs.
Check this code:
let round_id = *extra_chainlink_round_ids.at(i);
let round_data = chainlink_instance.round_data(round_id);
...
token_amount_claimable_from_last_claim += (*expanded_claim_bips_array.at(i))
.into() * actual_total_amount
* base.pow(chainlink_instance.decimals().into())
//@audit This is a user-provided input param, and the only check performed is that it is within the time tolerance limit
/ round_data.answer.into()
/ BIPS_PRECISION.into();
Imagine that a user who is entitled to claim supplies an extra_chainlink_round_ids array with round IDs pointing to rounds where the reported price ( round_data.answer) is lower. This would increase the computed token amount claimable because the division by a lower number will inflate the result.
Imagine that the tolerance limit is set to 5 minutes, and the token's price jumps 50% from 10$ -> 15$ within 5 minutes. The user can call the claim(...) function and provide the round_ids from when the token's price was still 10$, enabling them to claim more tokens than intended.
Recommendation
Consider using Chainlink's defined method of fetching historical data then validating the rounds with the fetched historical data.
Resolution
Acknowledged, but no fix.
6.4 One tolerance may not fit for all the unlock timestamps
While claiming tokens, in the case of Chainlink price feed is enabled, the users have to provide round IDs for each unlock timestamp. The timestamps corresponding to the round IDs should be under the tolerance limit w.r.t the corresponding unlock timestamps. The owner is supposed to set the tolerance limit to a lower value. Otherwise, users may take advantage of it by providing round IDs corresponding to the lowest price within the tolerance limit, thus claiming more tokens. However, setting it to a lower value might not fit all the unlock timestamps, as Chainlink price feed data might not be available within the tolerance limit for some timestamps. Plus, in a volatile market, the price may drop significantly for a shorter duration and again become stable. Still, users may use the round ID corresponding to that price drop event if that fits within the tolerance limit, getting a significant advantage.
Recommendation
Consider using Chainlink's defined method of fetching historical data then validating the rounds with the fetched historical data. Another way would be to consider the last few prices within the tolerance limit and average them so that one significant price drop for a smaller duration may not manipulate the token amount.
Resolution
Acknowledged, but no fix.
6.5 Unreached contract state in get_fee(...) function
The get_fee(...) function which is used in calculating the amount of fees on the amount of tokens to transfer to the fee collector checks if the custom_fee_bips is set. If the custom_fee_bips is zero, it uses the default_fee_bips. In the event the fee_bips is set equal to the BIPS_PRECISION, the fee_bips used is zero as seen below:
fn get_fee(
self: @ContractState, unlocker_instance: ContractAddress, tokens_transferred: u256
) -> u256 {
let mut fee_bips = self.custom_fee_bips.read(unlocker_instance);
if fee_bips == 0 {
// If the fee is unset, the default fee should be used.
fee_bips = self.default_fee_bips.read();
} else if fee_bips == BIPS_PRECISION { //@audit this state will not be reached given MAX_FEE check in setting custom fee and default
// If the fee is set to 100%, it means the fee is zero.
fee_bips = 0;
}
tokens_transferred * fee_bips / BIPS_PRECISION
}
The issue is either custom_fee_bips or default_fee_bips can never be set to BIPS_PRECISION given the MAX_FEE check in both set_default_fee(...) and set_custom_fee(...).
The constant BIPS_PRECISION = 10000 and MAX_FEE = 1000.
Recommendation
Consider updating the fee configuration mechanism to allow setting the fee to BIPS_PRECISION for the contract to reach that state.
6.6 Inconsistency between claim and delegate_claim
The claim(...) function returns the amount_claimed as a u256 parameter.
fn claim(
ref self: TContractState,
actual_id: u256,
claim_to: ContractAddress,
recipient_id: u64,
extra_data: Span<felt252>,
) -> u256;
The delegate_claim, on the other hand, does not return the amount_claimed.
fn delegate_claim(
ref self: TContractState, actual_id: u256, recipient_id: u64, extra_data: Span<felt252>,
);
Recommendation
Consider returning the amount_claimed in the delegate_claim(... function to maintain consistency.
