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

transfer() overwrites balanceChange #82

Open
kantp opened this issue Jul 2, 2024 · 0 comments
Open

transfer() overwrites balanceChange #82

kantp opened this issue Jul 2, 2024 · 0 comments
Labels
audit Addresses findings from the audit o1js Issues that require changes in o1js itself, rather than in this repository warning

Comments

@kantp
Copy link
Collaborator

kantp commented Jul 2, 2024

The TokenContract.transfer() function produces two account updates: one for the sender of the funds and one for the receiver.

Rather than creating the AccountUpdates from scratch, it can accept a from and to AccountUpdate. In this case, it overwrites the balanceChange field to the amount being transferred.

Snippet from transfer()

async transfer(
  from: PublicKey | AccountUpdate,
  to: PublicKey | AccountUpdate,
  amount: UInt64 | number | bigint
) {
  // [VERIDISE] ... elided

  from.balanceChange = Int64.from(amount).neg();
  to.balanceChange = Int64.from(amount);
  
  let forest = toForest([from, to]);
  await this.approveBase(forest);
}

However, from and to may have non-zero balanceChanges when passed to transfer(). Setting balanceChange overrides whatever the previous value was.

Impact

If users pass an AccountUpdate with an existing balance change to this function generated by another application, they may experience difficult-to-debug prover errors due to the mutation.

Recommendation

Use AccountUpdate.balance.addInPlace() and AccountUpdate.balance.subInPlace() instead of overwriting balances. Note that, if from and to have existing balances, this will require they are negatives of each other

@kantp kantp added audit Addresses findings from the audit warning o1js Issues that require changes in o1js itself, rather than in this repository labels Jul 2, 2024
@kantp kantp moved this to Todo in Fungible Token Audit Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit Addresses findings from the audit o1js Issues that require changes in o1js itself, rather than in this repository warning
Projects
Development

No branches or pull requests

1 participant