when add new yang , get_recent_asset_absorption_error() may reach recursion limit

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/absorber.cairo#L745

Vulnerability details

Vulnerability details

In update_absorbed_asset() it will use get_recent_asset_absorption_error() to get the most recent last_error. get_recent_asset_absorption_error() internally is a recursive call to the next absorption. Example: absorption_id = 1000

get_recent_asset_absorption_error(1000) -> get_recent_asset_absorption_error(999) .....get_recent_asset_absorption_error(0)

        fn get_recent_asset_absorption_error(self: @ContractState, asset: ContractAddress, absorption_id: u32) -> u128 {
            if absorption_id == 0 {
                return 0;
            }

            let absorption: DistributionInfo = self.asset_absorption.read((asset, absorption_id));
            // asset_amt_per_share is checked because it is possible for the error to be zero.
            // On the other hand, asset_amt_per_share may be zero in extreme edge cases with
            // a non-zero error that is spilled over to the next absorption.
            if absorption.asset_amt_per_share.is_non_zero() || absorption.error.is_non_zero() {
                return absorption.error;
            }

            self.get_recent_asset_absorption_error(asset, absorption_id - 1)
        }

Normally, it will not recursively call to absorption_id=0, because in most cases absorption.asset_amt_per_share or absorption.error will have values.

But there is one exception: for the newly added yang, the old absorption's asset_amt_per_share/error will both be 0, causing it to recurse from 1000 to 0.

get_recent_asset_absorption_error(1000) -> .....get_recent_asset_absorption_error(0)

Most languages have a recursion limit.

Python's default is 1000, I'm not sure what cairo's is.

I tried testing to 15000 and it gave an error.

Could not reach the end of the program. RunResources has no remaining steps

Proof of Concept

add to test_absorber.cairo
    #[test]
    fn test_recall_recent() {      
        let absorption_id:u32 = 15000;
        let (_, _, _, absorber, _, _) = absorber_utils::absorber_deploy(
            Option::None, Option::None, Option::None, Option::None, Option::None, Option::None
        );        
        absorber.get_absorption_error(absorption_id);
    }

add to absorber.cairo

    impl IAbsorberImpl of IAbsorber<ContractState> {
        fn get_absorption_error(ref self: ContractState, absorption_id: u32) -> u128 {
            return self.get_recent_asset_absorption_error(get_contract_address(),absorption_id);
        }    

$ scarb test -vvv test_recall_recent

[FAIL] opus::tests::absorber::test_absorber::test_absorber::test_recall_recent

Failure data:
    Got an exception while executing a hint: Hint Error: Error in the called contract (0x041f7a65be670b7019cd46fcc710d8eea72e610b66c3cf9bdfddced306e6adf8):
Error at pc=0:16838:
Could not reach the end of the program. RunResources has no remaining steps.
Cairo traceback (most recent call last):
Unknown location (pc=0:13917)
Unknown location (pc=0:13917)

Impact

Exceeding the recursion limit causes an error, or generates a large GAS.

Recommended Mitigation

get_recent_asset_absorption_error () add max recursion

-       fn get_recent_asset_absorption_error(self: @ContractState, asset: ContractAddress, absorption_id: u32) -> u128 {
+       fn get_recent_asset_absorption_error(self: @ContractState, asset: ContractAddress, absorption_id: u32 , max_recursion:u32) -> u128 {
-           if absorption_id == 0 {            
+           if absorption_id == 0 || max_recursion == 0{
                return 0;
            }

            let absorption: DistributionInfo = self.asset_absorption.read((asset, absorption_id));
            // asset_amt_per_share is checked because it is possible for the error to be zero.
            // On the other hand, asset_amt_per_share may be zero in extreme edge cases with
            // a non-zero error that is spilled over to the next absorption.
            if absorption.asset_amt_per_share.is_non_zero() || absorption.error.is_non_zero() {
                return absorption.error;
            }

-           self.get_recent_asset_absorption_error(asset, absorption_id - 1)
+           self.get_recent_asset_absorption_error(asset, absorption_id - 1 , max_recursion - 1)
        }

        fn update_absorbed_asset(
            ref self: ContractState, absorption_id: u32, total_recipient_shares: Wad, asset_balance: AssetBalance
        ) {
..
-           let last_error: u128 = self.get_recent_asset_absorption_error(asset_balance.address, absorption_id);
+           let last_error: u128 = self.get_recent_asset_absorption_error(asset_balance.address, absorption_id , 100);

Assessed type

Error

[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) confirmed

[c4-sponsor]

tserg marked the issue as disagree with severity

[tserg]

The likelihood of hitting the recursion limit is low given that the recursion limit is unclear and with reference to Liquity, which has had 878 stability pool liquidations since its inception, and Opus will likely have less because of searcher liquidations.

[alex-ppg]

The Warden has outlined a way in which the code may come close to the recursion limit of the Cairo language due to an asset having no absorption errors to carry over.

I believe that the likelihood of such an instance is very low as it assumes that the arithmetic calculations have all been carried out perfectly and that the absorption ID has become so high that the recursion limit can be breached.

A risk level of QA (NC) is better suited for this particular case, and there are many ways this particular issue can be mitigated.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Overinflated severity