ERC777 reentrancy allows attacker to drain reward in Absorber

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

The reap_helper(...) function in the Absorber is used to calculate and transfer the absorbed assets and rewards to the provider. It's triggered whenever a provider takes any action.

However, this function doesn't follow the Check-Effects-Interaction (CEI) pattern. As the code snippet below shows, it first calculates the rewards, transfers the rewards to the provider, and then updates the provider's cumulative rewards.

If the token is ERC777 or any token with a call hook to the receiver, an attacker could call back to the Absorber and drain the reward token. This is possible because the cumulative rewards are updated after token transfers.

fn reap_helper(ref self: ContractState, provider: ContractAddress, provision: Provision) {
    ...
    let (absorbed_assets, rewarded_assets) = self
        .get_absorbed_and_rewarded_assets_for_provider(provider, provision, false);

    // Get and update provider's absorption ID
    self.provider_last_absorption.write(provider, self.absorptions_count.read());

    // Loop over absorbed and rewarded assets and transfer
    self.transfer_assets(provider, absorbed_assets);

    // @audit ERC777 reentrancy. Provider's cumulative rewards are updated later
    self.transfer_assets(provider, rewarded_assets); 

    ...
    self.update_provider_cumulative_rewards(provider);

    self.emit(Reap { provider, absorbed_assets, reward_assets: rewarded_assets });
}

Proof of Concept

The Absorber has multiple epochs. A new epoch is created when the amount of yin per share drops below a threshold. However, for simplicity, we'll consider only one epoch which is the current epoch in the scenario below:

1. In the current epoch, assume asset_amt_per_share = 1000 and provider_last_reward_cumulative = 0 is the attacker's last reward cumulative. The attacker has epoch_shares = 1000 shares in the current epoch.

2. The attacker calls reap(), which in turn calls reap_helper(). This function calls get_provider_rewards() to calculate the provider reward using the code snippet below. The result is reward_amt = 1000 * (1000 - 0) = 1e6.

   let rate: u128 = if epoch == provision.epoch {
   asset_amt_per_share - self.provider_last_reward_cumulative.read((provider, reward.asset))
   } else {
   asset_amt_per_share
   };
   reward_amt += u128_wmul(rate, epoch_shares.val);

3. The reap_helper() function calls transfer_assets() to transfer 1e6 reward to the attacker. The token has a call hook to the receiver/attacker address, allowing the attacker to take control and call reap() again.

4. Since provider_last_reward_cumulative is not updated, the same calculation occurs, and the contract continues to transfer 1e6 reward to the attacker. Attacker can repeat these steps until the pool is empty.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a reentrancy guard to Absorber and follow the CEI pattern in the reap_helper() function.

Assessed type

Reentrancy

[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]

Tokens with callbacks are fundamentally incompatible and should not be onboarded in the first place according to the docs.

[alex-ppg]

The Warden specifies how ERC-777 assets may cause misbehavior in the Opus protocol, however, as the Sponsor correctly specifies the onboarding documentation that was linked to during the contest specifies a clear list of requirements for collaterals and does not indicate support for these types of tokens.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid