User can frontrun calls to `set_share` function to gain more shares.

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

The set_share function presumably allows the admin to set the number of a user's shares.

    pub fn set_share(who: &T::AccountId, pool: &T::PoolId, new_share: T::Share) {
        let (share, _) = Self::shares_and_withdrawn_rewards(pool, who);

        if new_share > share {
            Self::add_share(who, pool, new_share.saturating_sub(share));
        } else {
            Self::remove_share(who, pool, share.saturating_sub(new_share));
        }
    }

The issue with the function is that due to the comparisons of the new_share with old share, a user can frontrun the call, to initially remove his shares, and the turning his shares_and_withdrawn_rewards to 0, and then backrunning the call to add his previously removed shares. If the pool is the NATIVE currency pool, the user can simply unbond and rebond once the admin's call is executed.

Because the user has removed his shares, his share is now 0, thus, the new_share will always be more, thus calling the add_share function and adding the new_share to his account, getting way more shares that is desired.

Proof of Concept

Two test cases are shown below, copy and paste them into tests.rs

1. Test one shows a user with 100000 shares having his shares set to 100 normally
2. Test two shows a user removing shares just before the set_shares is called, and then adding shares after and having his shares increased.

fn set_shares_without_frontrun() {
    ExtBuilder::default().build().execute_with(|| {
        assert_eq!(RewardsModule::pool_infos(DOT_POOL), Default::default());
        assert_eq!(
            RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, ALICE),
            Default::default()
        );
        RewardsModule::add_share(&ALICE, &DOT_POOL, 100000);
        assert_eq!(
            RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, ALICE),
            (100000, Default::default())
        );
        RewardsModule::set_share(&ALICE, &DOT_POOL, 100);

        assert_eq!(
            RewardsModule::pool_infos(DOT_POOL),
            PoolInfo {
                total_shares: 100,
                ..Default::default()
            }
        );
        assert_eq!(
            RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, ALICE),
            (100, Default::default())
        );
    });
}

fn set_shares_frontrun() {
    ExtBuilder::default().build().execute_with(|| {
        assert_eq!(RewardsModule::pool_infos(DOT_POOL), Default::default());
        assert_eq!(
            RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, ALICE),
            Default::default()
        );
        RewardsModule::add_share(&ALICE, &DOT_POOL, 100000);

        RewardsModule::remove_share(&ALICE, &DOT_POOL, 99999);
        assert_eq!(
            RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, ALICE),
            (1, Default::default())
        );
        RewardsModule::set_share(&ALICE, &DOT_POOL, 100);

        assert_eq!(
            RewardsModule::pool_infos(DOT_POOL),
            PoolInfo {
                total_shares: 100,
                ..Default::default()
            }
        );
        assert_eq!(
            RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, ALICE),
            (100, Default::default())
        );
        RewardsModule::add_share(&ALICE, &DOT_POOL, 99999);
        assert_eq!(
            RewardsModule::shares_and_withdrawn_rewards(DOT_POOL, ALICE),
            (100099, Default::default())
        );
    });
}

Tools Used

Manual code review

Recommended Mitigation Steps

Instead of directly setting shares, use an increase/decrease shares functions instead. A pause functionality can also be implemented to temporarily halt shares removal just before the shares can be set.

Assessed type

Other

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[DadeKuma]

This issue is similar to the approve race condition which is capped to NC/Invalid according to the SC: https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-approve-race-condition-suggested-nc-or-invalid

The mitigation does not solve the issue as it can be front-run anyway.

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid

[OpenCoreCH]

Hey @ZanyBonzy,

The issue is conceptually similar to the approval frontrunning issue in the sense that in both cases a user A (here the admin) decreases a balance (here shares, in the approval frontrunning an approval which can be used to transfer tokens) and the user could frontrun this call to benefit from the old balance. That being said, Invalid was a bit harsh because the issue is technically valid. I just do not think that it is a valid medium because the function works as intended. If the admin really wanted to increase or decrease by a certain value (which is not clear to me that this will ever be needed), they could do this via a small helper contract that queries the balance and increases / decreases it in the same tx. But like you said, in the case of decreasing, the only improvement in the case of decreasing would be that the call reverts, materially nothing is changed. Maybe that would help the admin to detect this behaviour, but they could also detect it easily with the current implementation (in the end, all transactions are public).

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-a