Debt from a liquidated trove may not be redistributed due to an issue inside the function `shrine.redistribute_helper()`

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L1881-L1887 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L1995-L2005 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/types.cairo#L92-L95 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L2086

Vulnerability details

Impact

The function shrine.redistribute_helper() may wrongly calculate a liquidated trove's debt amount to be redistributed to other troves to be 0. Thus no debt may be redistributed to other troves upon liquidations.

Details:

• Inside the function shrine.redistribute_helper() on line 1881 in shrine.cairo, redistributed_yang_unit_debt may be computed to be 0 in the case where the denominator redistributed_yang_recipient_pool is bigger than the numerator adjusted_debt_to_distribute_for_yang in the division, due to rounding down to 0 in fixed-point arithmetic.

• redistributed_yang_unit_debt (YangRedistribution.unit_debt) is the amount of debt in wad to be distributed to each wad unit of yang, quoting the comment on line 93-94 inside types.cairo.

• redistributed_yang_unit_debt is subsequently assigned to YangRedistribution.unit_debt upon defining the variable redistributed_yang_info (line 1995 shrine.cairo).

• Subsequently a new redistribution entry is written into self.yang_redistributions (line 1998-2000 shrine.cairo), where redistributed_yang_info is wrongly holding the value 0 for unit_debt, thus no debt will be redistributed.

• Note: The calculation for debt to redistribute relies on unit_debt. Debt is redistributed whenever a deposit/withdrawal/forge/melt tx happens in the shrine, for example in this trace: shrine.deposit()->shrine.charge()->shrine.pull_redistributed_debt_and_yangs(). There inside shrine.pull_redistributed_debt_and_yangs() on line 2086 in shrine.cairo the computation of the debt to redistribute is done by using unit_debt.

Example:

• total troves: 100
• 1 trove is liquidated/redistributed
• each trove has 10 yang deposited, total yang = 1000 yang
• liquidated user's debt: 10 yin (exchange rate yang/yin 1:1)
• yang pool of all other troves: 990 yang
• The shrine then calculates (line 1881 shrine.cairo):
• adjusted_debt_to_distribute_for_yang = 10 yin
• redistributed_yang_recipient_pool = 990 yang
• redistributed_yang_unit_debt = 10 / 990 = 0

Impact:

• Debt from a liquidated trove may not be distributed to other troves, leading to an issue, where other troves may receive yang from the liquidated trove without being accounted with additional debt. This may lead to a loss for the protocol, and to a skewed system state that may introduce further issues.

• An attacker may intentionally set up an unhealthy trove together with multiple other troves to make profit, since the unhealthy trove is only distributing it's yang to the other troves upon liquidation but not it's debt, thus resulting in a profit for the attacker and a loss for the protocol. The attacker may repeat this process to drain a vast amount of assets from the protocol.

Note that this issue is not a division before multiplication issue, because division before multiplication is used to truncate the debt_error.

Similar issues in codebase:

• Line 1941-1942 in shrine.cairo: The calculation of unit_yang has the same issue.
• Line 1950-1951 in shrine.cairo: The calculation of unit_debt has the same issue.

Proof of Concept

POC: https://gist.github.com/zzzitron/202f6b2aedef776fbcd9d8225b36b5fd

This POC is almost identical to the existing test_shrine_redistribute_dust_yang_rounding test. The difference is only the deposited yang1 amount of TROVE_1, and an added assertion that shows that yang1 unit debt is wrongly 0 which makes the assertion and the test fail.

POC steps:

1. Redistribute
2. Show that yang1 unit debt is zero. The added assertion makes the test fail since the yang1 unit debt is wrongly zero.

In the POC this assertion checks whether yang1 unit debt is not zero and fails:

// test_shrine_redistribution.cairo

        assert(
            shrine.get_redistribution_for_yang(yang1_addr, expected_redistribution_id).unit_debt.is_non_zero(),
            'audit: wrong unit debt'
        );

Note that when adjusting the test by making the yang1 deposit amount for TROVE_1 smaller (using TROVE1_YANG1_DEPOSIT), that all of a sudden the test passes successfully, and yang1 unit debt is indeed not zero. This shows that when other depositors deposited a high enough amount, then the denominator redistributed_yang_recipient_pool in the division line 1881-1882 shrine.cairo will be too big resulting in the rounding down to 0.

Note that the yang1 deposit of the redistributed_trove in this POC is 1_000 Wad (trove2_yang1_amt), which is why the yang1 deposit of TROVE_1 had to be set to 10_000 (CUSTOM_TROVE1_YANG1_DEPOSIT) so that the issue arises. If the yang1 deposit of the redistributed_trove would be for example only 1 Wad than the deposit of the other trove doesn't have to be 10_000 Wad and instead it may be enough to be 10 Wad. Or as an alternative there may be 10 other troves where each of these troves deposited 1 Wad so that the sum of the other deposits is 10 Wad in that case.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adjusting the calculations by adding precision:

// shrine.cairo
+1880                           let precision = 10**18;
1881                            redistributed_yang_unit_debt = adjusted_debt_to_distribute_for_yang * precision
1882                                / redistributed_yang_recipient_pool; // @audit applying precision
1883
1884                            // Due to loss of precision from fixed point division, the actual debt distributed will be less than
1885                            // or equal to the amount of debt to distribute.
1886                            let actual_debt_distributed: Wad = redistributed_yang_unit_debt
1887                                * redistributed_yang_recipient_pool / precision; // @audit normalizing precision
1888                            debt_error = adjusted_debt_to_distribute_for_yang - actual_debt_distributed;

Assessed type

Math

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-sponsor]

tserg (sponsor) disputed

[tserg]

"redistributed_yang_unit_debt = 10 / 990 = 0": This is wrong because 10 is first scaled up by wad i.e. the suggested fix is already inherent in the WadDiv implementation.

Also, if redistributed_yang_uni_debt is zero, then actual_debt_distributed on the next line would also be zero, and all the debt would go towards the debt_error instead.

Lastly, in the POC test, the unit debt for yang1 is not zero.

[alex-ppg]

The Warden specifies how truncation may lead to errors that would round values to 0, however, these truncations cannot occur as the Warden has failed to detect the types involved in the arithmetic operations are Wad.

As we can observe in the relevant library implementation, the native *, /, etc. arithmetic operators are overloaded and performed using the usual WadRayMath notation which would ensure they are properly scaled and do not round down as described.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid