Fix MayUseToken vulnerability in AccountUpdates #1716
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ent are not both true The isParentsOwnToken method now checks that inheritFromParent is false when validating if an account update is for a parent's own token. This ensures that an account update cannot have both parentsOwnToken and inheritFromParent set to true at the same time. Additionally, a new check method is added to the mayUseToken object to assert that parentsOwnToken and inheritFromParent are not both true. This provides an extra layer of validation to catch any invalid combinations of these flags. These changes prevent inconsistent states and improve the integrity of the account update data structure.
…ayUseToken Add unit tests to cover the following cases: - Correctly identifies parentsOwnToken and not inheritFromParent - Correctly identifies inheritFromParent and not parentsOwnToken - Throws an error when both parentsOwnToken and inheritFromParent flags are true in the check method - Correctly identifies when neither parentsOwnToken nor inheritFromParent flag is set The tests ensure that the AccountUpdate.MayUseToken methods correctly handle different combinations of the parentsOwnToken and inheritFromParent flags, improving the robustness and reliability of the AccountUpdate functionality.
…untUpdates - Modified isParentsOwnToken() to return false when inheritFromParent is true to avoid misuse of token inheritance - Added check() method to ensure parentsOwnToken and inheritFromParent flags are not simultaneously set to true
MartinMinkov
force-pushed
the
audit/may-use-token
branch
from
5142caa to
32ba4cc
Compare
Trivo25
reviewed
Jul 5, 2024
There was a problem hiding this comment.
Fix looks good to me, but I think this is a breaking change since isParentsOwnToken did do what it said it would originally, but could be misleading if used improperly. Imo we should consider adding isParentsOwnTokenV2 and potentially defering the well formedness check in check to v2 entirely
mitschabaude
reviewed
Jul 5, 2024
src/lib/mina/account-update.ts
Outdated
Comment on lines
1204
to
1211
| check(mayUseToken: { parentsOwnToken: Bool; inheritFromParent: Bool }) { | ||
| // Ensure the flags are not both set to true | ||
| mayUseToken.parentsOwnToken | ||
| .and(mayUseToken.inheritFromParent) | ||
| .assertFalse( | ||
| 'MayUseToken: parentsOwnToken and inheritFromParent cannot both be true' | ||
| ); | ||
| }, |
There was a problem hiding this comment.
there are a couple of issues with this check() method:
- it's not in the right place, so wouldn't be called automatically to constrain a
MayUseTokentype when we witness it- it needs to go into the provable type definition that lives on
.type:{ ...provablePure(...), check() { ... } }
- it needs to go into the provable type definition that lives on
MayUseToken.typeis not even the provable type that the auto-derived account update type will use -- it treats themayUseTokenproperty as an anonymous object of two bools, seegen/transactions.tsandjs-layout.ts. So, when witnessing entire account updates, nothing preventsmayUseTokenfrom having both props = true- Even if we modified the account update type, there are many places where we skip all checks on witnessed account updates, on purpose, because of the assumption that well-formedness of them is asserted by the transaction snark on the Mina node. (see
AccountUpdate.witness()with theskipCheckparameter). From the finding, it wasn't clear to me if the OCamlmayUseTokenfield really includes such a wellformedness check. If not, I think we should add it in the necessary places in o1js somehow, for example in the token contract where it walks child account updates.
There was a problem hiding this comment.
Got it! Do these changes make sense then?
Constructing the check method properly and changing the AccountUpdate.check method to also check for mayUseToken?
EDIT: Had to add this change as well c177ddf
Doesn't seem to like my check method! https://github.com/o1-labs/o1js/actions/runs/9882602168/job/27295800852?pr=1716#step:4:201
MartinMinkov
changed the title
…efff582ad95e663cc
…eck for inheritFromParent flag The isParentsOwnToken method was checking that inheritFromParent is not true when validating if an account update is for a parent's own token. However, this check is unnecessary as the two flags are mutually exclusive and should not both be true at the same time. The check method was ensuring this invariant. By removing the inheritFromParent check from isParentsOwnToken and the entire check method, we simplify the code while still maintaining the intended behavior. The isParentsOwnToken method now only returns the value of the parentsOwnToken flag, and the isInheritFromParent method continues to return the value of the inheritFromParent flag. This refactoring improves code readability and maintainability by removing redundant checks and simplifying the logic.
MartinMinkov
force-pushed
the
audit/may-use-token
branch
from
48ba34d to
df603bc
Compare
…ons to the top-level scope The MayUseToken type and its related functions were previously defined within the AccountUpdate class. Moving them to the top-level scope improves code organization and makes them more accessible. The MayUseToken type is now imported from the '../../bindings/mina-transaction/transaction-leaves.js' module as BaseMayUseToken and then extended with additional properties and functions. This change enhances code readability and maintainability by separating concerns and promoting a more modular structure.
…ate.check method to ensure parentsOwnToken and inheritFromParent are not both true This change adds an additional check in the AccountUpdate.check method to validate that the mayUseToken field has valid values. The check ensures that parentsOwnToken and inheritFromParent flags are not both set to true, as that would be an invalid state. This fix helps maintain data integrity and catches potential issues early. test(account-update.unit-test.ts): add test case for AccountUpdate.check method to verify it throws an error when both parentsOwnToken and inheritFromParent are true A new test case has been added to verify that the AccountUpdate.check method correctly throws an error when both parentsOwnToken and inheritFromParent flags are set to true in the mayUseToken field. This test helps ensure the fix in the AccountUpdate.check method is working as expected and prevents invalid states.
…inition refactor(account-update.ts): simplify MayUseToken type by using BaseMayUseToken directly test(account-update.unit-test.ts): update MayUseToken type check to use AccountUpdate.MayUseToken.type.check instead of AccountUpdate.MayUseToken.check The changes remove the provablePure wrapper from the MayUseToken type definition and simplify it by using the BaseMayUseToken directly. This is done to fix an issue and simplify the type definition. The unit test is also updated to use the correct type check method after the refactoring.
Comment on lines
+1200
to
+1203
| static check = (au: AccountUpdate) => { | ||
| Types.AccountUpdate.check(au); | ||
| Types.MayUseToken.check(au.body.mayUseToken); | ||
| }; |
There was a problem hiding this comment.
this override is not necessary, Types.AccountUpdate.check() already works by recursively calling check on all the types it is composed of
|
Closed for #1741 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR addresses a security vulnerability in the
MayUseTokenfield of AccountUpdate. The issue could potentially lead to miscalculation of total balance changes due to improper handling of token inheritance.Key changes:
Changes
These changes prevent potential misuse of the API where a parent AccountUpdate might incorrectly treat a child as using the parent's own derived token when it's actually inheriting the token ID or using the default token ID.
Repo updates:
bindings: o1-labs/o1js-bindings#279