Acala - AM's results

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L326-L361

Vulnerability details

Impact

Accounts can use the function transfer_share_and_rewards, to transfer share and the underlying rewards for that amount of shares from one account to another, the formula used inside it can be reduce to (TOTAL_REWARDS * shares_to_transfer) / TOTAL_SHARES ) however if TOTAL_SHARES is bigger then (TOTAL_REWARDS * shares_to_transfer ) OR vice-versa then the result is rounded down to 0 and the transfer of shares will happen but there will be no transfer of underlying rewards so the amount of shares that the sender have will change but the amount of rewards will not and in the balance of the receiver he will receive shares but no underlying rewards.

Proof of Concept

We wrote a unit test, to run the unit test use the following command

cargo test --release  --package orml-rewards --lib -- tests::test_transfer_share_and_rewards_finding_rounding_down_in_rewards_formula_shares_gets_transfered_and_rewards_not

The unit test:

fn test_transfer_share_and_rewards_finding_rounding_down_in_rewards_formula_shares_gets_transfered_and_rewards_not(){
	ExtBuilder::default().build().execute_with(|| {
		RewardsModule::add_share(&ALICE, &DOT_POOL, 100);
		assert_eq!(
			RewardsModule::pool_infos(DOT_POOL),
			PoolInfo {
				total_shares: 100,
				rewards: Default::default()
			}
		);

		assert_ok!(RewardsModule::accumulate_reward(&DOT_POOL, NATIVE_COIN, 100));
		assert_eq!(
			RewardsModule::pool_infos(DOT_POOL),
			PoolInfo {
				total_shares: 100,
				rewards: vec![(NATIVE_COIN, (100, 0))].into_iter().collect()
			}
		);

		RewardsModule::add_share(&BOB, &DOT_POOL, 100);

		assert_eq!(
			RewardsModule::pool_infos(DOT_POOL),
			PoolInfo {
				total_shares: 200,
				rewards: vec![(NATIVE_COIN, (200, 100))].into_iter().collect()
			}
		);

		assert_ok!(RewardsModule::transfer_share_and_rewards(&ALICE, &DOT_POOL, 33, &BOB));

		assert_eq!(
			RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, ALICE),
			(67, Default::default())
		);

		assert_eq!(
			RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, BOB),
			(133, vec![(NATIVE_COIN, 100)].into_iter().collect())
		);

		assert_ok!(RewardsModule::transfer_share_and_rewards(&BOB, &DOT_POOL, 1, &CAROL));
		assert_eq!(
			RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, CAROL),
			(1, vec![(NATIVE_COIN, 100 * 1 / 133)].into_iter().collect())
		);

		assert_eq!(
			RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, BOB),
			(132, vec![(NATIVE_COIN, 100)].into_iter().collect())
		);

		assert_eq!(
			RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, CAROL),
			(1, vec![(NATIVE_COIN, 0)].into_iter().collect())
		);
	})
}

Tools Used

Manual Review

Recommended Mitigation Steps

Check for rounding inside the math operation and revert the transaction if happens

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L157 https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L179 https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L216 https://github.com/code-423n4/2024-03-acala/blob/9c71c05cf2d9f0a2603984c50f76fc8a315d4d65/src/orml/rewards/src/lib.rs#L273

Vulnerability details

Impact

DoS can be achieved over principal flows like adding shares, removing shares, claiming rewards and so on... thanks to an iteration over a mapper that holds no entries boundaries.

Proof of Concept

PoolInfos and SharesAndWithdrawnRewards mappers holds the info about CurrencyId and Balances in a BTreeMap as there could be different balance amount for each currency, however in the flows of add_share, remove_share, claim_rewards and others the logic iterates over all the entries, if the numbers of entries will be to big it will cause the main functions we iterate over above to fail with out of gas as there are is not enough resources to iterate over all of them.

Tools Used

Manual Review

Recommended Mitigation Steps

Limit the number of Currency's the mappers can hold in the BTree.

Assessed type

DoS