Lack of total outstanding rewards in the contract which can prevent any unclaimed reward mistakenly removed

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x080ec85127c3fbad2166363af2075849afbb3c43543d08a932c85318df0fdf1c Severity: minor

Description: Description

Currently there is no total outstanding rewards in the contract. This total outstanding reward is basically the total sum of any outstanding_rewards for every member_id in every committee_id.

In Most, there is a recover_azero which can recover any azero inside the contract, but in my opinion the function need to be restricted to transfer amount in the contract excluding this total outstanding rewards. This is because the member_id still have rights to request payout_rewards which still inside the contract.

        /// Transfer AZERO tokens from the bridge contract to a given account.
        ///
        /// Can only be called by the contracts owner
        #[ink(message)]
        pub fn recover_azero(
            &mut self,
            receiver: AccountId,
            amount: u128,
        ) -> Result<(), MostError> {
            self.ensure_owner()?;

            self.env().transfer(receiver, amount)?;
            Ok(())
        }

Another assumption is, if recover_azero is to recover pocket_money_balance, then the pocket_money_balance need to be updated its value. So instead using recover_azero to remove pocket_money_balance it's more appropriate to create a pocket_money_withdrawal.

Other issue, the way pocket_money_balance works, allowing a non revert if the transfer fails, and then the pocket_money_balance is still deducted by pocket_money, can make the pocket_money_balance unsynced. This might be a different issue, but I merge into this one because it's also a minor issue which can affecting internal accounting balance of azero inside the contract.

                // bootstrap account with pocket money
                if data.pocket_money_balance >= data.pocket_money {
                    // don't revert if the transfer fails
                    _ = self
                        .env()
                        .transfer(dest_receiver_address.into(), data.pocket_money);
                    data.pocket_money_balance = data
                        .pocket_money_balance
@>                      .checked_sub(data.pocket_money)
                        .ok_or(MostError::Arithmetic)?;
                }

I submit this as minor issue, because it need to have an edge case situation which might rarely happen. It's depends on protocol to implement this or not, but this is still a minor issue.

Noticing from Discord, a low/minor issue can use detailed/step-by-step worded PoC, as follows:

Attack Scenario

Detailed step-by-step PoC:

1. A committee #1 (with 5 member) get_collected_committee_rewards for example, have 10.000 AZERO
2. A committee #2 (with 5 member) have 20.000 AZERO
3. Alice, which is a member from committee #1 and #2, never request her rewards. Meanwhile other members already claimed. This makes Alice can claim 6000 AZERO, this is what I called, total outstanding reward, because only alice hasn't claim.
4. Since there is no total outstanding reward, it's hard to determine how many AZERO is safe to recover, without impacting alice unclaimed reward.
5. If the recover_azero turns out eat some of alice's unclaimed rewards, alice may failed to claim her reward (temporary)
6. Imagine if there are several committee established, and several members have not request their rewards, thus accounting of total_unclaimed_reward might necessary if not a informational statistics

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Recommendation

Consider to implement a total_outstanding_rewards which updates on request rewards and send_request, and prevent any recover_azero taking this total outstanding amount.

[krzysztofziobro]

Valid submission - minor