Misuse of balance lock

[sameh-farouk]

Describe the bug

By reviewing the billing code, it seems that the balance lock feature (from the balances pallet) may have been used incorrectly in our smart-contract pallet (at least it wasn't intended for such a use case).

Here are some side effects that I can think of:

• I have observed that at times, we unintentionally lock amounts more than the due one.
• While checking if a contract should be moved into the grace period or restored, we do not consider the previously locked amount. This can lead to the contract going into the grace period earlier than expected or requiring additional funds to get restored.
• There is a possibility that this issue may cause failure while distributing rewards.
• Even after canceling all contracts and paying all due amounts still leave part of the user balance locked unexpectedly.

For the last point, my account on Devnet 5DFkH2fcqYecVHjfgAEfxgsJyoEg5Kd93JFihfpHDaNoWagJ can serve as an example.

[
  {
    id: 0x0000000000000050
    amount: 631,048,769
    reasons: Misc
  }
  {
    id: 0x000000000000005a
    amount: 640,655,139
    reasons: Misc
  }
  {
    id: 0x0000000000000077
    amount: 613,120,026
    reasons: Misc
  }
  {
    id: 0x000000000000007c
    amount: 615,344,631
    reasons: Misc
  }
  {
    id: 0x000000000000007f
    amount: 619,049,311
    reasons: Misc
  }
  {
    id: 0x0000000000000082
    amount: 634,682,950
    reasons: Misc
  }
  {
    id: 0x0000000000000464
    amount: 8,818,612,335
    reasons: Misc
  }
  {
    id: 0x00000000000002ed
    amount: 5,334,075,143
    reasons: Misc
  }
  {
    id: 0x0000000000000a82
    amount: 841,878
    reasons: Misc
  }
  {
    id: 0x0000000000000613
    amount: 23,429,034
    reasons: Misc
  }
  {
    id: 0x00000000000006d2
    amount: 73,693,350
    reasons: Misc
  }
  {
    id: 0x00000000000002eb
    amount: 47,553,021
    reasons: Misc
  }
  {
    id: 0x00000000000002f0
    amount: 53,892,981
    reasons: Misc
  }
  {
    id: 0x0000000000000dde
    amount: 3,467
    reasons: Misc
  }
  {
    id: gridlock
    amount: 8,819,115,335
    reasons: All
  }
]

There are two issues here: the remaining locks from an old billing implementation that weren't properly removed, and the discrepancy involving having 881.9 TFT locked with gridlock ID, even though I have no contracts at all.

I can provide more details later (issue to be updated)

Update:
I would recommend rewriting the locking code to use holds instead of locks. Holds are very similar to locks, with two major differences:

• Locks can overlap, whereas reserves/holds are additive.

Example:

• Apply Hold to account A for 20 units, then apply another Hold to account A for 10 units → a total of 30 units will be reserved/hold
• Apply Lock to account A for 20 units, then apply another Lock to account A for 10 units → a total of 20 units will be locked, since 10 is smaller than 20.

In our code, we are trying to add locks in a way that is similar to how reserve/hold works. This is the reason why it should be replaced by holds instead.

[0oM4R]

I'm working on threefoldtech/tfgrid-sdk-ts#2489 , and while testing the flow, i had total locked amount with less than 19 tft,

balance details :

i have funded my wallet with 19 tft, i was expecting to bill all of them, but after call billContractForBlock for all contracts, all contracts moved back to created state, I my balance details :

locks like there is somthing wrong with rent contract billing

[sameh-farouk]

Due to a bug ...
Here we check if the user has the amount due for the current cycle plus any existing amount in the contract lock.

tfchain/substrate-node/pallets/pallet-smart-contract/src/billing.rs

Line 166 in 5a9ad37

let contract = Self::handle_grace(&mut contract, usable_balance, lock_amount)?;

This leads to the contract being restored if this amount is available.
When it comes to locking the funds in the user's balance, we only lock the amount due from the last cycle.

tfchain/substrate-node/pallets/pallet-smart-contract/src/billing.rs

Line 185 in 5a9ad37

Self::handle_lock(contract, &mut contract_lock, amount_due)?;

This should fixed in this PR , which kinda WIP