Sweat EconomyDefer Feature Security Review

We've observed that the claim function uses the 'is_locked' field in the AccountRecord as a race condition lock to prevent issues such as double spending. However, despite the account being locked prior to making a cross-contract call, the lock is not asserted at the beginning of the function, rendering it ineffective.

sweat-claim:claim:contract/src/claim/api.rs

            fn claim(&mut self) -> PromiseOrValue<ClaimResultView> {
              let account_id = env::predecessor_account_id();

require!( self.is_claim_available(account_id.clone()) == ClaimAvailabilityView::Available, "Claim is not available at the moment" );

             let account_data = self.accounts.get_mut(&account_id).expect("Account data is
     not found");
            account_data.is_locked = true;

Fortunately, even without enabled lock, no race condition occurred because accruals were cleared before a cross-contract call. Therefore, even if the function is called again, the user doesn't have any accrued balance to claim. This was tested using both a batch call through near_workspaces and through a smart contract.

poc:batch

            #[tokio::test]
            async fn race_condition_exploit() -> anyhow::Result<()> {
              let mut context = prepare_contract().await?;

              let alice = context.alice().await?;
              let manager = context.manager().await?;

             let alice_steps = 10_000;
             let alice_initial_balance =
     context.ft_contract().ft_balance_of(alice.to_near()).call().await?;

             let target_token_amount = context.ft_contract().formula(U64(0),
     alice_steps).call().await?.0;
             let target_fee = target_token_amount * 5 / 100;
             let target_effective_token_amount = target_token_amount - target_fee;

context .ft_contract() .defer_batch(vec![(alice.to_near(), alice_steps)], context.sweat_claim().account()) .with_user(&manager) .call() .await?;

              let claim_contract_balance = context
                .ft_contract()
                .ft_balance_of(context.sweat_claim().account())
                .call()
                .await?;

poc:batch

assert_eq!(claim_contract_balance.0, target_effective_token_amount);

               let alice_deferred_balance = context
                 .sweat_claim()
                 .get_claimable_balance_for_account(alice.to_near())
                 .call()
                 .await?;
               assert_eq!(alice_deferred_balance.0, target_effective_token_amount);

alice.batch(&context.sweat_claim().account().to_string().parse()?) .call(Function::new("claim").args_json(json!({}))) .call(Function::new("claim").args_json(json!({}))) .transact().await?.into_result()?;

               let alice_balance = context.ft_contract().ft_balance_of(alice.to_near()).call().await?;

               let alice_balance_change = alice_balance.0 - alice_initial_balance.0;

assert_eq!(alice_balance_change, target_effective_token_amount);

               Ok(())
           }

poc:exploit_smart_contract

            #[tokio::test]
            async fn race_condition_exploit() -> anyhow::Result<()> {
              let mut context = prepare_contract().await?;

              let exploit_contract = context.worker.clone();

              let exploit_contract = exploit_contract.dev_deploy(EXPLOIT_CONTRACT).await?;

context .ft_contract() .storage_deposit(Some(exploit_contract.id().clone().parse()?), None) .call() .await?; context .ft_contract() .tge_mint(&exploit_contract.id().clone().parse()?, U128(100_000_000)) .call() .await?;

              let alice = exploit_contract.id().to_string().parse::<AccountId>()?;
              let manager = context.manager().await?;

             let alice_steps = 10_000;
             let alice_initial_balance =
     context.ft_contract().ft_balance_of(alice.clone()).call().await?;

             let target_token_amount = context.ft_contract().formula(U64(0),
     alice_steps).call().await?.0;
             let target_fee = target_token_amount * 5 / 100;
             let target_effective_token_amount = target_token_amount - target_fee;

poc:exploit_smart_contract

context .ft_contract() .defer_batch(vec![(alice.clone(), alice_steps)], context.sweat_claim().account()) .with_user(&manager) .call() .await?;

            let claim_contract_balance = context
              .ft_contract()
              .ft_balance_of(context.sweat_claim().account())
              .call()
              .await?;

assert_eq!(claim_contract_balance.0, target_effective_token_amount);

            let alice_deferred_balance = context
              .sweat_claim()
              .get_claimable_balance_for_account(alice.clone())
              .call()
              .await?;

assert_eq!(alice_deferred_balance.0, target_effective_token_amount);

exploit_contract.call("call_claim").args_json(json!({"addr": context.sweat_claim().account()})).max_gas() .transact().await?.into_result()?;

            let alice_balance = context.ft_contract().ft_balance_of(alice).call().await?;
            let alice_balance_change = alice_balance.0 - alice_initial_balance.0;
            assert_eq!(alice_balance_change, target_effective_token_amount);

            Ok(())
        }

source:exploit_smart_contract

          #[near_bindgen]
          impl Contract {
            pub fn call_claim(addr: AccountId) -> Promise {
              Promise::new(addr.clone())
                .function_call(
                   "claim".into(),
                   json!({}).to_string().as_bytes().to_vec(),
                   0,
                   (ONE_TERRA * 100).into(),
                )
                .and(Promise::new(addr).function_call(
                   "claim".into(),
                   json!({}).to_string().as_bytes().to_vec(),
                   0,
                   (ONE_TERRA * 100).into(),
                ))
                .then(
                   Self::ext(env::current_account_id())
                      .with_static_gas((ONE_TERRA * 30).into())
                      .resolve_callback(),
                )}
            pub fn resolve_callback(#[callback_result] result: Result<ClaimResultView,
     PromiseError>) {
              match result {
                Ok(result) => {
                   log!("Success: {:#?}", result);
                }
                Err(e) => {
                   log!("Error: {:?}", e);
                }
              }
            }
          }

PROPOSED SOLUTION

We propose adding an assertion in the beginning of the claim function

REMEDIATION - FIXED

The Sweat Foundation team has fixed the issue by adding an assertion in the beginning of the claim
function in this commit: 4077a6d51baad63a1bbb8bbfc7b34d6e2f70c9c3

We've noticed that under certain conditions, the defer batch may fail with the production batch size. The backend utilizes a batch size of 135 when invoking the defer function in the FT smart contract. After FT makes a cross-contract call to the claim smart contract to record steps, it generates a log by pushing the (account_id, amount) tuples to the amounts field of the RecordData structure, for each batch entry. This pattern carries a risk of exceeding the log size limit, which could cause a transaction failure. Additionally, no tests use the actual production batch size. Therefore, while increasing the batch size might pass the tests, transactions could still fail in practice due to exceeding prepaid gas or the log size limit.

sweat_claim:record_batch_for_hold:contract/src/record/api.rs

            let mut event_data = RecordData {
               timestamp: now_seconds,
              amounts: vec![],
            };
            for (account_id, amount) in amounts {
              event_data.amounts.push((account_id.clone(), amount));

              let amount = amount.0;
              let index = balances.len();

total_balance += amount; balances.push(amount); ...

emit(EventKind::Record(event_data));

We conducted tests with a constant number of 10,000 steps, which emulates a possible number of steps for each entry. We used accounts of varying lengths to determine the minimum account length required for a batch of 135 entries to fail. After testing, we found that the minimum account length is 57 characters, plus 5 characters from the .near suffix. This means if the backend sends a batch of 135 entries, each with an account length of 62 characters, the transaction will fail due to the log size limit.

poc:defer_max

            #[tokio::test]
            async fn defer_max() -> anyhow::Result<()> {
              let mut context = prepare_contract().await?;

                let manager = context.manager().await?;

                let long_account = format!("{}.near", "a".repeat(57))
                     .parse::<AccountId>()?;

                let vec = vec![(long_account, 10000); 135];

context .ft_contract() .defer_batch(vec, context.sweat_claim().account()) .with_user(&manager) .call() .await?;

                Ok(())
            }

PROPOSED SOLUTION

The likelyhood of this issue occurring in production is low, but we still propose to add a test with the production batch size to ensure that the issue does not occur in the future.

REMEDIATION - FIXED

The Sweat Foundation team has fixed the issue by adding a test with the production batch size in this commit: 09aa633d7dd408fa6ee8a6fee5604d7be8d85c1f

During token calculations, it was observed that the oracle fee is rounded down. When the sweat_to_mint value is less than 20, the oracle fee is 0. In all other cases, the oracle fee is rounded down, which results in a cumulative loss for the protocol (even if it is insignificant). As SWEAT FT has 18 decimals, having a sweat_to_mint value of less than 20 is rare. However, it is recommended to always round against the user to prevent cumulative loss and any potential system exploitation.

sweat-near:calculate_tokens_amount:contract/src/record/api.rs

            pub(crate) fn calculate_tokens_amount(&self, steps: u32) -> (u128, u128) {
              let sweat_to_mint: u128 = self.formula(self.steps_since_tge, steps).0;
             let trx_oracle_fee: u128 = sweat_to_mint * 5 / 100;
              let minted_to_user: u128 = sweat_to_mint - trx_oracle_fee;

                (minted_to_user, trx_oracle_fee)
            }

PROPOSED SOLUTION

We propose to use div_ceil to round up the oracle fee.

REMEDIATION - FIXED

The Sweat Foundation team has fixed the issue by using div_ceil to round up the oracle fee in this commit: e208f3a06050637c6477c73e06cd69a241fe5d7d

It was observed that the on_record callback uses a custom check to determine if the signer is an oracle to restrict access. However, this callback should not be directly accessed by anyone. Additionally, the signer account isn't utilized anywhere beyond the initial check. To avoid this and restrict the callback to the contract only, the callback should be marked as #[private]. This approach is more conventional and clarifies the intention.

sweat-near:on_record:contract/src/record/api.rs

          fn on_record(&mut self, receiver_id: AccountId, amount: U128, fee_account_id:
     AccountId, fee: U128) {
           if !self.oracles.contains(&env::signer_account_id()) {
               panic_str("The operation can be only initiated by an oracle");
            }

                if !is_promise_success() {
                    panic_str("Failed to record data in holding account");
                }

                let mut events: Vec<FtMint> = Vec::with_capacity(2);
                ...
            }

PROPOSED SOLUTION

We propose to use #[private] macro to restrict accesss to the callback.

REMEDIATION - FIXED

The Sweat Foundation team has fixed the issue by using #[private] macro in this commit: 7bbae6072f8b79fe92c9fff1770233bdb30a06e2