Acala - TheSchnilch's results

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/modules/earning/src/lib.rs#L188-L196

Vulnerability details

Description

With unbond_instant, a user can unbond a bonded amount directly without having to wait. However, they must pay a fee for this:

let amount = change.change;
let fee = fee_ratio.mul_ceil(amount);
let final_amount = amount.saturating_sub(fee);

let unbalance = T::Currency::withdraw(&who, fee, WithdrawReasons::TRANSFER, ExistenceRequirement::KeepAlive)?;
T::OnUnstakeFee::on_unbalanced(unbalance);

T::OnUnbonded::happened(&(who.clone(), final_amount));

Here, the mistake is that T::OnUnbonded::happened is called with final_amount, meaning without a fee. As a result, a portion of the shares that were added as shares during bonding can no longer be removed. This, in turn, leads to these shares still receiving rewards that other users cannot receive.

If you insert a println! statement into the functions bond and unbond_instant to display the amounts with which OnBonded::happened and OnUnbonded::happened are called, you will see when you execute the following code that not all shares are removed when the bonded amount of a user is removed with unbond_instant:

For bond:

+ println!("change.change: {:?}", change.change);
144: T::OnBonded::happened(&(who.clone(), change.change));
145: Self::deposit_event(Event::Bonded {
146: 	who,
147: 	amount: change.change,
148: });

For unbond_instant:

+ println!("final_amount: {:?}", final_amount);
196: T::OnUnbonded::happened(&(who.clone(), final_amount));
197: Self::deposit_event(Event::InstantUnbonded {
198: 	who,
199: 	amount: final_amount,
200: 	fee,
201: });

Code that can be inserted into the file modules/earning/src/tests.rs to test both functions:

#[test]
fn poc() {
	ExtBuilder::default().build().execute_with(|| {
		assert_ok!(Earning::bond(RuntimeOrigin::signed(ALICE), 100)); //100 tokens are bonded. change.change is 100

		assert_ok!(Earning::unbond_instant(RuntimeOrigin::signed(ALICE), 100)); //All bonded tokens will be unbonded immediately but only 90 shares will be removed because of the fee amount.
	});
}

The code can be started with the command cargo test poc -- --nocapture

So every time a user calls unbond_instant, some shares will remain in the reward system and continue to accumulate rewards that other users who actually bond can no longer get. These shares can then no longer be removed and are stuck. With a maximum fee of 20%, some shares could accumulate over a period of time and these shares would accumulate rewards. The owners of the shares can still claim these rewards. In addition, one share is no longer the same as one underlying token.

Recommendation

For unbond_instant, the final_amount should not be used to remove the shares, but change.change should be used instead.

Assessed type

Other