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

Fix MayUseToken vulnerability in AccountUpdates #1716

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Fix reversed order of account updates when using `TokenContract.approveAccountUpdates()` https://github.com/o1-labs/o1js/pull/1722
- Fixed the static `check()` method in Struct classes to properly handle inheritance, preventing issues with under-constrained circuits. Added error handling to avoid using Struct directly as a field type. https://github.com/o1-labs/o1js/pull/1707
- Fixed that `Option` could not be used as `@state` or event https://github.com/o1-labs/o1js/pull/1736
- Address potential incorrect token inheritance handling with `MayUseToken` in AccountUpdates https://github.com/o1-labs/o1js/pull/1716
- Modified isParentsOwnToken() to return false when inheritFromParent is true, preventing potential misuse of token inheritance.
- Added check() method to ensure parentsOwnToken and inheritFromParent flags are not both set to true simultaneously.

## [1.4.0](https://github.com/o1-labs/o1js/compare/40c597775...ed198f305) - 2024-06-25

Expand Down
63 changes: 32 additions & 31 deletions src/lib/mina/account-update.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { Memo } from '../../mina-signer/src/memo.js';
import {
Events as BaseEvents,
Actions as BaseActions,
MayUseToken as BaseMayUseToken,
} from '../../bindings/mina-transaction/transaction-leaves.js';
import { TokenId as Base58TokenId } from './base58-encodings.js';
import {
Expand Down Expand Up @@ -131,8 +132,6 @@ type AuthRequired = Types.Json.AuthRequired;
type AccountUpdateBody = Types.AccountUpdate['body'];
type Update = AccountUpdateBody['update'];

type MayUseToken = AccountUpdateBody['mayUseToken'];

type Events = BaseEvents;
const Events = {
...BaseEvents,
Expand All @@ -158,6 +157,32 @@ const Actions = {
},
};

type MayUseToken = BaseMayUseToken;
const MayUseToken = {
type: BaseMayUseToken,
No: {
parentsOwnToken: Bool(false),
inheritFromParent: Bool(false),
},
ParentsOwnToken: {
parentsOwnToken: Bool(true),
inheritFromParent: Bool(false),
},
InheritFromParent: {
parentsOwnToken: Bool(false),
inheritFromParent: Bool(true),
},
isNo: ({
body: {
mayUseToken: { parentsOwnToken, inheritFromParent },
},
}: AccountUpdate) => parentsOwnToken.or(inheritFromParent).not(),
isParentsOwnToken: (accountUpdate: AccountUpdate) =>
accountUpdate.body.mayUseToken.parentsOwnToken,
isInheritFromParent: (mayUseToken: AccountUpdate) =>
mayUseToken.body.mayUseToken.inheritFromParent,
};

/**
* Either set a value or keep it the same.
*/
Expand Down Expand Up @@ -675,6 +700,7 @@ class AccountUpdate implements Types.AccountUpdate {

static Actions = Actions;
static Events = Events;
static MayUseToken = MayUseToken;

constructor(body: Body, authorization?: Control);
constructor(body: Body, authorization: Control = {}, isSelf = false) {
Expand Down Expand Up @@ -1171,7 +1197,10 @@ class AccountUpdate implements Types.AccountUpdate {
static empty() {
return AccountUpdate.dummy();
}
static check = Types.AccountUpdate.check;
static check = (au: AccountUpdate) => {
Types.AccountUpdate.check(au);
Types.MayUseToken.check(au.body.mayUseToken);
};
Comment on lines +1200 to +1203
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this override is not necessary, Types.AccountUpdate.check() already works by recursively calling check on all the types it is composed of

static fromFields(fields: Field[], [other, aux]: any[]): AccountUpdate {
let accountUpdate = Types.AccountUpdate.fromFields(fields, aux);
return Object.assign(
Expand Down Expand Up @@ -1204,34 +1233,6 @@ class AccountUpdate implements Types.AccountUpdate {
return Provable.witnessAsync(combinedType, compute);
}

static get MayUseToken() {
return {
type: provablePure({ parentsOwnToken: Bool, inheritFromParent: Bool }),
No: { parentsOwnToken: Bool(false), inheritFromParent: Bool(false) },
ParentsOwnToken: {
parentsOwnToken: Bool(true),
inheritFromParent: Bool(false),
},
InheritFromParent: {
parentsOwnToken: Bool(false),
inheritFromParent: Bool(true),
},
isNo({
body: {
mayUseToken: { parentsOwnToken, inheritFromParent },
},
}: AccountUpdate) {
return parentsOwnToken.or(inheritFromParent).not();
},
isParentsOwnToken(a: AccountUpdate) {
return a.body.mayUseToken.parentsOwnToken;
},
isInheritFromParent(a: AccountUpdate) {
return a.body.mayUseToken.inheritFromParent;
},
};
}

/**
* Returns a JSON representation of only the fields that differ from the
* default {@link AccountUpdate}.
Expand Down
73 changes: 73 additions & 0 deletions src/lib/mina/account-update.unit-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
Mina,
Int64,
Types,
Bool,
} from '../../index.js';
import { Test } from '../../snarky.js';
import { expect } from 'expect';
Expand All @@ -20,6 +21,14 @@ function createAccountUpdate() {
return accountUpdate;
}

function createAccountUpdateWithMayUseToken(
mayUseToken: AccountUpdate['body']['mayUseToken']
) {
let accountUpdate = AccountUpdate.defaultAccountUpdate(address);
accountUpdate.body.mayUseToken = mayUseToken;
return accountUpdate;
}

// can convert account update to fields consistently
{
let accountUpdate = createAccountUpdate();
Expand Down Expand Up @@ -122,3 +131,67 @@ function createAccountUpdate() {
'Check signature: Invalid signature on fee payer for key'
);
}

// correctly identifies parentsOwnToken and not inheritFromParent
{
let accountUpdate = createAccountUpdateWithMayUseToken({
parentsOwnToken: Bool(true),
inheritFromParent: Bool(false),
});
expect(
AccountUpdate.MayUseToken.isParentsOwnToken(accountUpdate).toBoolean()
).toEqual(true);
expect(
AccountUpdate.MayUseToken.isInheritFromParent(accountUpdate).toBoolean()
).toEqual(false);
}

// correctly identifies inheritFromParent and not parentsOwnToken
{
let accountUpdate = createAccountUpdateWithMayUseToken({
parentsOwnToken: Bool(false),
inheritFromParent: Bool(true),
});
expect(
AccountUpdate.MayUseToken.isParentsOwnToken(accountUpdate).toBoolean()
).toEqual(false);
expect(
AccountUpdate.MayUseToken.isInheritFromParent(accountUpdate).toBoolean()
).toEqual(true);
}

// throws an error when both flags are true in check method
{
expect(() => {
AccountUpdate.MayUseToken.type.check({
parentsOwnToken: Bool(true),
inheritFromParent: Bool(true),
});
}).toThrowError(
'MayUseToken: parentsOwnToken and inheritFromParent cannot both be true'
);
}

// throws an error when both flags are true in AccountUpdate.check method
{
let accountUpdate = createAccountUpdateWithMayUseToken({
parentsOwnToken: Bool(true),
inheritFromParent: Bool(true),
});
expect(() => {
AccountUpdate.check(accountUpdate);
}).toThrowError(
'MayUseToken: parentsOwnToken and inheritFromParent cannot both be true'
);
}

// correctly identifies when neither flag is set
{
let accountUpdate = createAccountUpdateWithMayUseToken({
parentsOwnToken: Bool(false),
inheritFromParent: Bool(false),
});
expect(AccountUpdate.MayUseToken.isNo(accountUpdate).toBoolean()).toEqual(
true
);
}
Loading