Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contracts which invoke the balances are not acid compliant #242

Open
bassjobsen opened this issue Aug 5, 2018 · 1 comment
Open

Contracts which invoke the balances are not acid compliant #242

bassjobsen opened this issue Aug 5, 2018 · 1 comment
Assignees
Labels

Comments

@bassjobsen
Copy link
Contributor

Version

1.4

Environment

linux

Steps to reproduce

from https://github.com/AschPlatform/cctime/blob/master/contract/cctime.js#L90 the code (in summary) looks like that shown below:

    let balance = app.balances.get(this.trs.senderId, VOTE_CURRENCY)
    app.balances.decrease(this.trs.senderId, VOTE_CURRENCY, amount)
    app.balances.increase(article.authorId, VOTE_CURRENCY, authorReward)

What is expected?

Something like that shown beneath:

    let balance = app.balances.get_for_update(this.trs.senderId, VOTE_CURRENCY)
    // get_for_update should lock the `balance` table
    app.balances.starttranaction()
      app.balances.decrease(this.trs.senderId, VOTE_CURRENCY, amount)
      app.balances.increase(article.authorId, VOTE_CURRENCY, authorReward)
   app.balances.commit()

also see: https://sqlite.org/lang_transaction.html

What is actually happening?

In the current situation you can not ensure that the balance does still contain enough assets when app.balances.decrease() run actually.
Also we got some troubles when app.balances.decrease() fails and app.balances.decrease() did not fail or visa versa.

@bassjobsen
Copy link
Contributor Author

bassjobsen commented Aug 5, 2018

In fact the main problem seems to be that one shouldn't not be able to invoke a increase or decrease of the balance at all. A contract should always do a transfer (from the balance of the sender to the balance of the receiver).
transfer() should check if the balance of the sender contains enough assets. So i suggest to replace https://github.com/AschPlatform/asch-core/blob/master/src/smartdb/balance-manager.js with the code shown beneath:

function getCurrencyFlag(currency) {
  if (currency === 'XAS') {
    return 1
  } if (currency.indexOf('.') !== -1) {
    // UIA
    return 2
  }
  // gateway currency
  return 3
}

// "protected" functions
increase(address, currency, amount) {
  if (app.util.bignumber(amount).eq(0)) return
  const key = { address, currency }
  let item = this.sdb.get('Balance', key)
  if (item) {
    item.balance = app.util.bignumber(item.balance).plus(amount).toString(10)
    app.sdb.update('Balance', { balance: item.balance }, key)
   } else {
    item = this.sdb.create('Balance', {
      address,
      currency,
      balance: amount,
      flag: getCurrencyFlag(currency),
    })
  }
}

decrease(address, currency, amount) {
  this.increase(address, currency, `-${amount}`)
}  

class BalanceManager {
  constructor(sdb) {
    this.sdb = sdb
  }

  get(address, currency) {
    const item = this.sdb.get('Balance', { address, currency })
    const balance = item ? item.balance : '0'
    return app.util.bignumber(balance)
  }

  increase(address, currency, amount) {
    return 'You can only increase a balance by doing a transfer'
  }

  decrease(address, currency, amount) {
    return 'You can only decrease a balance by doing a transfer'
  }

  transfer(currency, amount, from, to) {
    app.sdb.beginTrans()
    if(this.get(from, currency) < amount) return 'not enough' + currency + 'to transfer'
    decrease(from, currency, amount)
    increase(to, currency, amount)
    app.sdb.commit() 
  }
}

module.exports = BalanceManager

Notice that the above should not only fix the current issue, but also #229

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants