Potential DOS when the number of redistributions is excessively large

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/04583e0411dbf8027952d668a8678fda0cb5b160/src/core/shrine.cairo#L2040

Vulnerability details

Impact

Whenever users interact with their troves, the pull_redistributed_debt_and_yangs() function is called to calculate the amount of debts and yangs pulled after redistribution. This function contains three nested loops, which consume a significant amount of gas. If the max transaction size is exceeded, the transaction will revert.

Currently the block limit on mainnet is 10M steps but the max transaction size is only 3M steps. https://docs.starknet.io/documentation/tools/limits_and_triggers/

Because any user actions with troves require the pull_redistributed_debt_and_yangs() call, users could be prevented from interacting with their troves.

Proof of Concept

This function operates by looping from the last redistribution of the trove to the current redistribution. In each redistribution, it loops through each yang. For each yang, if it's an exceptional redistribution, another loop through all the yangs will occur.

Therefore, in the worst-case scenario, the function's complexity is O(n * m^2), where n is the number of redistributions and m is the number of yangs.

If the number of redistributions becomes too large, the number of Cairo steps required to execute pull_redistributed_debt_and_yangs() will exceed the limit, resulting in a DOS for users.

Tools Used

Manual Review

Recommended Mitigation Steps

Introduce another function that allows users to partially pull redistributed debts and yangs. For instance, if there are 10 redistributions, users could pull 5 redistributions in one transaction, and the remaining 5 in the next. However, all other actions should only be executed after all redistributions are pulled.

Assessed type

DoS

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[alex-ppg]

The Warden specifies that if too many redistributions occur for a particular trove, it may not be able to catch up due to hitting the step limit of the Starknet blockchain.

This submission is better suited as a QA (NC) recommendation to introduce a mechanism to gradually catch up through redistributions for a particular trove. A scenario in which a trove will not be able to catch up is very unlikely given that it means the trove was inactive (all operations will force it to catch up) for multiple distributions.

In reality, a single distribution will in turn incentivize the user to operate on their trove to avoid their position going underwater due to f.e. a liquidated collateral that is volatile. A scenario in which thousands of redistributions will occur with no trove activity is not going to manifest in a production environment.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c