Update the state of `data.pocket_money_balance` in `most/lib.rs` only if transfer succeeded.

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0xeeabb0d4d50ad5e0083f4eab2c380e6d1c5f3b95092c77205b2e395219460f42 Severity: low

Description: Description\

In azero/contracts/most/lib.rs#L431-L439:

  // 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)?;
                }

If the transfer failed in the self.env().transfer() function, the code expects to continue with the execution. However, the code doesn't handle properly as when the transfer failed, the variable data.pocket_money_balance still get deducted regardless.

Attack Scenario\

If the transfer failed, the variabledata.pocket_money_balance still deducted regardless. As a result, this will cause this variable to return lesser values then expected.

Attachments

NA

1. Proof of Concept (PoC) File

Manual Analysis

We can see that the return value of self.env().transfer() function is saved in _ variable which is ignored.

2. Revised Code File (Optional)

Suggest to make the following changes:

// 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)?;
++                 let result = self.env().transfer(dest_receiver_address.into(), data.pocket_money);
++                 match result {
++                    Ok(()) => {
++                    // only update pocket_money_balance state if transfer succeeded.
++                    data.pocket_money_balance = data
++                    .pocket_money_balance
++                    .checked_sub(data.pocket_money)
++                    .ok_or(MostError::Arithmetic)?
++                     }

++                   Err(_) => {
++                   // don't revert if the transfer fails
++                   }
++               }

                }

[krzysztofziobro]

Valid submission - minor (puts contract in an unexpected state)

[krzysztofziobro]

Noticed not that this is a duplicate of the issue included in #33