Closed c4-bot-8 closed 9 months ago
The given example appear to be under the impression that a redistribution is either ordinary or exceptional for all yangs. Instead, ETH and BTC from trove[1] will be ordinarily redistributed, and USDC will be exceptionally redistributed.
In any event, the trove yangs' balances need to be updated when exceptionally redistributed yangs are received because these can in turn receive ordinary redistributions from later redistributions.
[EDIT - fixed typo]
bytes032 marked the issue as insufficient quality report
The Warden has showcased how the same loop performed in the same redistribution ID will use a trove's updated balance from a previous loop iteration rather than the Warden's balance from before the start of the loop.
The maximum impact of this exhibit would be a slightly skewed distribution, and, based on the fact that exceptional and ordinary distributions can be "mixed-and-matched" per the Sponsor's comments, the present approach of the code would be considered correct as balances need to be updated per distribution iteration. As such, I consider this submission to be invalid.
alex-ppg marked the issue as unsatisfactory: Invalid
@tserg hi,Please take a look at this issue again, thank you.
Each round of revision needs to be reflected in the next round, which is correct.
The question is, in the same round of user acquisition calculation (amount*perShares), this amount
should use the amount at the beginning of this round, and should not use the amount increased by another asset in this round, because the value distributed to users at the time of distribution is not included.
For example, as mentioned in the example:
In the same round:
when pulling btc (after pull eth), the amount
should not include yang_increment_for_eth
(it from the same round "pull eth" )
it should still use the value at the beginning of this round, that is, 10, not 20.
pull eth:
yang_increment_for_eth = trove[2].btc * yang_to_yang_redistribution[btc,eth].unit_yang = 10 * 1 = 10
pull btc:
yang_increment_for_btc = (trove[2].eth + yang_increment_eth) * yang_to_yang_redistribution[eth,btc].unit_yang = (10 + 10) * 1 = 20
The correct one should be:
yang_increment_for_btc = trove[2].eth * yang_to_yang_redistribution[eth,btc].unit_yang =10 * 1 = 10
Example:
redistribution_id :1 => eth_amount =10 , btc_amount = 10
eth_amount * pershares
, not new_eth_amount * persharesredistribution_id :2 => eth_amount =20 , btc_amount = 20 (eth_amount,btc_amount all updated)
Hello @bin2chen66 @tserg @alex-ppg, I'd like to contribute to this discussion. I also considered this scenario during the audit, but found it to be essentially infeasible.
For two assets (such as eth and btc) to be redistributed in a single exceptional redistribution round, it would require a trove X to hold both assets exclusively, correct? However, when trove X undergoes redistribution, the recipient_yang_recipient_pool
for these assets would be 0
(as the balance from the redistributed trove is excluded), resulting in yang_to_yang_redistribution[eth, btc] = 0
. It means yang_increment_for_eth = 0
so it does not matter to use previous balance or new balance.
@minhquanym Oh, what you said seems reasonable. Let me think about it. thanks
@minhquanym This is a bit complicated. Please help me to see if there is any problem with my description below. Thank you.
The key to your example is whether yang_to_yang_redistribution[eth, btc] is 0 when distributing it.
The judgment condition of is ordinal redistribution is > = min _ recipient _ pool _ yang
= = wad one.
Ignoring is indeed `recipient yang recipient pool.is _ zero ()'
See the following code:
fn redistribute_helper(
ref self: ContractState,
redistribution_id: u32,
trove_id: u64,
debt_to_redistribute: Wad,
pct_value_to_redistribute: Ray,
current_interval: u64
) {
....
let is_ordinary_redistribution: bool =
@> redistributed_yang_recipient_pool >= MIN_RECIPIENT_POOL_YANG
.into();
if is_ordinary_redistribution {
....
} else {
// Keep track of the actual debt and yang distributed to calculate error at the end
// This is necessary for yang so that subsequent redistributions do not accrue to the
// earlier redistributed yang amount that cannot be attributed to any troves due to
// loss of precision.
let mut actual_debt_distributed: Wad = WadZeroable::zero();
let mut actual_yang_distributed: Wad = WadZeroable::zero();
let mut trove_recipient_yang_balances = trove_yang_balances;
// Inner loop over all yangs
loop {
match trove_recipient_yang_balances.pop_front() {
Option::Some(recipient_yang) => {
// Skip yang currently being redistributed
if *recipient_yang.yang_id == yang_id_to_redistribute {
continue;
}
let recipient_yang_initial_amt: Wad = self
.initial_yang_amts
.read(*recipient_yang.yang_id);
// Get the total amount of recipient yang that will receive the
// redistribution, which excludes
// (1) the redistributed trove's deposit; and
// (2) initial yang amount.
let recipient_yang_recipient_pool: Wad = *yang_totals
.at(*recipient_yang.yang_id - yang_id_to_array_idx_offset)
.amount
- *recipient_yang.amount
- recipient_yang_initial_amt;
// Skip to the next yang if no other troves have this yang
@> if recipient_yang_recipient_pool.is_zero() {
continue;
}
So suppose there is [usdc,eth,btc] usdc_total_shares = 100 usdc_alice_shares = 99.1 usdc_bob_shares = 0.9 eth_total_shares = 100 eth_alice_shares = 99.1 eth_bob_shares = 0.9 btc_total_shares = ... btc_alice_shares =.. btc_bob_shares = ...
when redistribute() , usdc,eth
, will exceptional redistributions ( 0.9 < MIN_RECIPIENT_POOL_YANG)
but:
usdc_for_eth_pre_shares >0 (because recipient_yang_recipient_pool.is_zero() is false)
eth_for_usc_pre_shares >0 (because recipient_yang_recipient_pool.is_zero() is false)
when bob pull redistribute: (usdc_bob_shares = 0.9, eth_bob_shares = 0.9)
Hey @bin2chen66, thanks for providing your feedback. I believe the system behaves as expected in the scenarios mentioned, but I will invite the Sponsor to double-check.
Based on the follow-up post and further verification, this is a valid finding.
This bug will affect a recipient trove when a single redistribution has exceptional redistributions for at least two yangs, and such yangs must satisfy the following conditions; (1) the recipient pool for the yang is in the range 0 < recipient_yang_recipient_pool
< MIN_RECIPIENT_POOL_YANG ; and (2) the recipient trove has deposited at least two of such yangs.
We disagree with the severity as it arises only in a specific combination of circumstances that are each already unlikely on their own.
tserg (sponsor) confirmed
tserg marked the issue as disagree with severity
Based on information provided by both the Warden and the Sponsor, I would re-evaluate this submission as QA (L) and still not eligible for an HM reward. Examining the likelihood of this submission:
n
exceptionally redistributed yangs are deposited in a single trove, and the cumulative deposits of those yangs lead to exceptional redistributions (i.e. totalDepositsOfYang < MIN_RECIPIENT_POOL_YANG
) (Likelihood: Low)The impact of the submission is a simultaneous increase of the Yang and Debt that the trove has acquired as a result of the n > 1
distributions. This increase, however, will break the overall accounting of the redistribution system.
Specifically, the yang_to_yang_redistribution
value will be "outdated" in the sense that it would have been calculated with a Trove evaluation without the newly attributed exceptionally redistributed funds. This means that, cumulatively, the exceptional redistribution will lead to a higher amount of both collateral and debt distributed, the former of which will tap into other user funds when a withdrawal is attempted after the debt has been repaid, redistributed, etc.
The proportion of Yang units that were incorrectly accounted for is directly proportionate to the previous exceptional re-distributions whereas the actual number of Yang units is dictated by the total amount of Yang units in the redistributed Trove. Considering more than 2
exceptional redistributions in a single redistribution period is detached from reality, so we will entertain the scenario of 2
assets being exceptionally redistributed.
This scenario would result in a permanently corrupted system state and the level of corruption is not necessarily restrained by the MIN_RECIPIENT_POOL_YANG
limitation as the first exceptional redistribution might have increased the balance beyond MIN_RECIPIENT_POOL_YANG
arbitrarily depending on how many funds the recipient trove acquired as part of the previous redistribution iteration.
I believe that the impact can be considered high, however, the combination of conditions for this behavior to arise has an extremely low likelihood. The first condition cannot be manufactured and is very unlikely as it means a single trove had two collaterals that no other trove in the system possessed and was liquidated. The second condition can be manufactured but this assumption would be invalid, as it would infer a user made a minuscule deposit ahead of time without any other user doing so meaning that only one person would be aware of the vulnerability.
In a realistic scenario, a high-evaluation trove with two assets that no other Trove holds a balance greater than MIN_RECIPIENT_POOL_YANG
of being liquidated and leading to two exceptional redistributions in the same redistribution period does not have an acceptable likelihood. I commend the Warden's due diligence and the fact that they have identified a real flaw in the codebase, however, the flaw would not manifest with a high impact and as such is better suited as a QA (L) vulnerability.
As some final thoughts on this judgment, I would like to state that the original submission does not detail the above flaw precisely. The example of the original submission uses a single exceptional redistribution which is secure, is not worded adequately, and fails to articulate the flaw properly.
Additionally, the Warden's responses indicate that they were not aware of the MIN_RECIPIENT_POOL_YANG
limitation, and detected a valid exploitation path for the vulnerability post-submission. The impact chapter also detailed that only Yang would be unfairly acquired without acknowledging that debt would be acquired unfairly nor that the cumulative Yang redistributed would exceed the original trove's value and thus cause an accounting flaw, which is the impact that would cause this exhibit to go beyond an unfair distribution and tapping into other users' funds.
Based on the above, I believe that regardless of the actual severity assessment the submission should not be accepted as valid when evaluated from a C4 perspective. I thank everyone who aided with this issue and greatly appreciate your PJQA contribution!
alex-ppg marked the issue as unsatisfactory: Insufficient proof
This issue is indeed very convoluted and complicated Although it was not finally judged to be valid, it was not in vain for the sponsors to be aware of this issue Thank you, Judge alex-ppg and minhquanym.
Lines of code
https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L2162-L2177
Vulnerability details
Vulnerability details
in
pull_redistributed_debt_and_yangs()
If
is_exceptional_redistribution=true
, we calculate the deservedyang
based on therecipient_yang_balance.amount
at that time.yang_increment = recipient_yang_balance.amount * exc_yang_redistribution.unit_yang
The problem above is that
trove_recipient_yang_balances
has been changed in the sametmp_redistribution_id
, which is not the quantity whenredistribute()
started.For example: trove[1].eth = 100 trove[1].btc = 100 trove[1].usdc =100
trove[2].eth = 10 trove[2].btc = 10
total_yang_eth = 110 total_yang_btc = 110
only trove[1] and trove[2]
execute
redistribute(trove[1],10%)
so: yang_to_yang_redistribution[btc,eth].unit_yang = 100 10% / 10 = 1 yang_to_yang_redistribution[eth , btc].unit_yang = 100 10% / 10 = 1
Currently,
pull_redistributed_debt_and_yangs(trove[2])
implementation code will execute:pull eth: yang_increment_for_eth = trove[2].btc yang_to_yang_redistribution[btc,eth].unit_yang = 10 1 = 10
pull btc: yang_increment_for_btc = (trove[2].eth + yang_increment_eth) yang_to_yang_redistribution[eth,btc].unit_yang = (10 + 10) 1 = 20 The correct one should be: yang_increment_for_btc = trove[2].eth yang_to_yang_redistribution[eth,btc].unit_yang =10 1 = 10
yang_increment_for_btc get more 10.
Impact
may get more yang than deserved.
Recommended Mitigation
Assessed type
Error