Sweat EconomyBurn Model Changes Security Review

We observed that get_burn_rate uses floating point arithmetic in smart contracts, and then casts the result from a floating point number to a fixed point one. This approach is prone to errors as it could potentially cause truncation errors.

model:get_burn_rate:model/src/lib.rs

          #[allow(
             clippy::cast_possible_truncation,
             clippy::cast_precision_loss,
             clippy::cast_sign_loss
          )]
          pub fn get_burn_rate(balance: TokensAmount, burn_period: Duration) ->
     TokensAmount {
            (balance as f64 / f64::from(burn_period)).ceil() as u128
          }

PROPOSED SOLUTION

We propose using div_ceil which handles the division and the ceiling of the result, without utilizing floating point arithmetic.

REMEDIATION - FIXED

The Sweat Foundation team has fixed the issue by replacing the calculation with div_ceil in this commit: 18a8f9d7c064f6fe89960eb7e07e1b0333faabff

We noticed that the get_account_mut method not only gets the mutable reference to the account, but also inserts the account if it does not exist. This could lead to unexpected behavior and potential security vulnerabilities if this method is used without checking the implementation.

common:get_account_mut:contract/src/common/mod.rs

         fn get_account_mut(&mut self, account_id: &AccountId) -> &mut AccountRecord {
          if !self.contains_key(account_id) {
             self.insert(account_id.clone(),
     AccountRecordVersioned::new(now_seconds()));
           }

              let AccountRecordVersioned::V1(account) =
     self.get_mut(account_id).expect("Account not found");
              account
            }

PROPOSED SOLUTION

We propose removing an account creation functionality from get_account_mut and creating a separate function called get_or_insert_account_mut which will use get_account_mut.

REMEDIATION - FIXED

The Sweat Foundation team has fixed the issue by implementing suggested fix in these commits: 417d42ea895aaf9193501b3b33ccc8b8a91e35b1 a0a9e4057a833b2d60c12ee7038a556dc9e8edde

We noticed that in both claim and burn methods, the contract does not check whether there is enough prepaid gas for cross-contract calls and callbacks. This could lead to the "Exceeded the prepaid gas" errors if not enough gas was attached

PROPOSED SOLUTION

We propose checking whether env::prepaid_gas() is enough for the cross-contract call and callback before making a call.

REMEDIATION - FIXED

The Sweat Foundation team has fixed the issue by taking a difference between env::prepaid_gas and env::used_gas and asserting it in both claim and burn methods before cross-contract calls in this commit: 06c59cdb1476d173de96a68ff82c00d4bf17e8bf