Users can be Unfairly Punished for Failures External to them, such as Delays in Transaction Processing or Sequencer Crashes.

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

Users who create a request to withdraw their yin from the absorber can be unfairly penalized in cases where the sequencer crashes or delays the transaction processing (something that has already happened several times in the network) this can cause the users can't withdraw yin if the absorber uses this yin to liquidate troves while the users are unable to remove the yin because they are waiting more time than they should due to being penalized for the sequencer problems.

If users depend on this yin to repay a trove, they can be at risk of being liquidated if they are unable to withdraw the yin when they need it to repay the trove.

Proof of Concept

In the Absorber contract any user can deposit (Provide function) and withdraw (Remove function) Yin to earn rewards and a portion of the collateral absorbed tokens from the shrine, the users are allowed to deposit and withdraw Yin at any time they want, but they can be unfairly penalized for the sequencer problems and this can cause that users can't withdraw their yin when they need, because while the users are waiting for the unfair penalized time to elapse the absorber can use the ying they want to withdraw, thus leaving users without the possibility of withdrawing the yin they need.

Let's look at the scenario mentioned before:

1. A bunch of users provide yin to start earning rewards and collateral in the absorber contract. https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/absorber.cairo#L397-L429

   // Some parts of the code have been omitted for simplicity.
   
       fn provide(ref self: ContractState, amount: Wad) {
           // total shares is below the minimum initial shares.
           let (new_provision_shares, issued_shares) = self.convert_to_shares(amount, false);
   
           // If epoch has changed, convert shares in previous epoch to new epoch's shares
           let current_epoch: u32 = self.current_epoch.read();
           let converted_shares: Wad = self.convert_epoch_shares(provision.epoch, current_epoch, provision.shares);
   
           let new_shares: Wad = converted_shares + new_provision_shares;
           self.provisions.write(provider, Provision { epoch: current_epoch, shares: new_shares });
   
           // Update total shares for current epoch
           self.total_shares.write(self.total_shares.read() + issued_shares);
   
           let success: bool = self.yin_erc20().transfer_from(provider, absorber, amount.into());
           assert(success, 'ABS: Transfer failed');
   }

2. Now, some time passed the users earned some rewards, so some users want to withdraw all or just some part of their yin from the Absorber to use it in any other strategy, so they create a request to withdraw their yin calling the request function. https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/absorber.cairo#L441-L460

       fn request(ref self: ContractState) {
           let provider: ContractAddress = get_caller_address();
           assert_provider(self.provisions.read(provider));
   
           let request: Request = self.provider_request.read(provider);
           let current_timestamp: u64 = get_block_timestamp();
   
           let mut timelock: u64 = REQUEST_BASE_TIMELOCK;
           if request.timestamp + REQUEST_COOLDOWN > current_timestamp {
               timelock = request.timelock * REQUEST_TIMELOCK_MULTIPLIER;
           }
   
           let capped_timelock: u64 = min(timelock, REQUEST_MAX_TIMELOCK);
           self
               .provider_request
               .write(
                   provider, Request { timestamp: current_timestamp, timelock: capped_timelock, has_removed: false }
               );
           self.emit(RequestSubmitted { provider, timestamp: current_timestamp, timelock: capped_timelock });
       }

3. While the users are waiting for the REQUEST_VALIDITY_PERIOD the sequencer starts having problems and delaying the transaction on the network and eventually the sequencer crashes, this causes the users can't remove their yin during the Validity Period, so when the sequencer and the network reestablish, the REQUEST_VALIDITY_PERIOD is over and the users couldn't call the Remove function to claim their yin due to the sequencer fail.

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

        fn assert_can_remove(self: @ContractState, request: Request) {
            let shrine = self.shrine.read();
            // Removal is not allowed if Shrine is live and in recovery mode.
            if shrine.get_live() {
                assert(!shrine.is_recovery_mode(), 'ABS: Recovery Mode active');
            }

            assert(request.timestamp.is_non_zero(), 'ABS: No request found');
            assert(!request.has_removed, 'ABS: Only 1 removal per request');

            let current_timestamp: u64 = starknet::get_block_timestamp();
            let removal_start_timestamp: u64 = request.timestamp + request.timelock;
            assert(removal_start_timestamp <= current_timestamp, 'ABS: Request is not valid yet');
            assert(current_timestamp <= removal_start_timestamp + REQUEST_VALIDITY_PERIOD, 'ABS: Request has expired');
        }

4. Now, the users have 2 options:
   • wait for the REQUEST_COOLDOWN time to end to avoid being penalized or.
   • call the request function again and wait for the penalized time to end.

The problem here is that in the first request, users could withdraw their ying almost immediately, but in the second request they have to wait more time, and during this time the Absorber contract could use their yin to liquidate another trove, thus leaving the users unable to withdraw their yin.

Writing this as a medium because the users may be at risk of being liquidated if they cannot withdraw their yin during volatile market conditions or they may be losing opportunities because of an unfair penalization due to the sequencer problems.

Tools Used

Manual Code Review.

Recommended Mitigation Steps

Maybe the user shouldn't be penalized for not claiming their yin during the REQUEST_VALIDITY_PERIOD

Assessed type

Other

[bytes032]

I think QA but still leaving for judge review cause the warden put some effort.

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-sponsor]

tserg (sponsor) disputed

[tserg]

These are inherent risks of any on-chain activity, plus there would be bigger problems if such scenarios do occur.

[alex-ppg]

I commend the Warden's effort in detailing the potential misbehavior that arises from a time-based window for claims, however, this submission represents a systemic risk of on-chain activity and should have been detailed in an Analysis report instead.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid