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(zoe): assert that amountKeywordRecord is a copyRecord #4069

Merged
merged 3 commits into from
Nov 16, 2021

Conversation

katelynsills
Copy link
Contributor

@katelynsills katelynsills commented Nov 12, 2021

BREAKING CHANGE: amountKeywordRecord must be hardened before passing to zcfMint and zcfSeat methods

Note: because this is a breaking change, we may want to wait to merge until we have other Zoe input validation complete, so there's only one breaking change to Zoe.

closes: #4061

Description

This PR changes the code of zcfMint.burnLosses, zcfMint.mintGains, zcfSeat.incrementBy, and zcfSeat.decrementBy to assert that the user-defined amountKeywordRecord is actually a keyword record and coerce the amounts. This means checking that it is a copyRecord, among other things. The actual function used is coerceAmountKeywordRecord from cleanProposal.js.

coerceAmountKeywordRecord requires a hardened object, thus, this is a breaking change. Any amountKeywordRecord not hardened will cause the function to throw.

Security Considerations

This is part of the input validation that we need to do for Zoe, but not the entirety of it. #4068 is the input validation task covering all of Zoe.

Documentation Considerations

This is a breaking change that will effect dapps. We should explain the need to harden in our documentation.

Testing Considerations

No additional tests needed.

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 55 to 57
lenderSeat.incrementBy(
harden(repaySeat.decrementBy(harden({ Loan: debt }))),
);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally the creator of an object is the one to harden the object before releasing it. The typical and best case, as in the remaining harden here, is on the object literal itself. Otherwise, things should only be passing hardened objects to each other. The receiver should be defensively checking and rejecting, as incrementBy already does. It should not be doing its own defensive hardening, as that masks bugs in the source of the object. For example, if decrementBy returned an unhardened object, the defensive harden would mask the bug. In this case it is fine because decrementBy is not buggy in this way.

Suggested change
lenderSeat.incrementBy(
harden(repaySeat.decrementBy(harden({ Loan: debt }))),
);
lenderSeat.incrementBy(repaySeat.decrementBy(harden({ Loan: debt })));

Please correct all places where this PR makes this error.

This will also cause some error messages to revert to what they were before this PR. IMO the old error messages were better anyway, so this is welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not seeing how this will cause some error messages to revert. I would expect the effect to be the same.

But, I just removed all of the harden calls on outputs from incrementBy and decrementBy, since they are already hardened.

BREAKING CHANGE: must harden `amountKeywordRecord` before passing to ZCF objects
@katelynsills katelynsills added the automerge:squash Automatically squash merge label Nov 16, 2021
@mergify mergify bot merged commit fe9a9ff into master Nov 16, 2021
@mergify mergify bot deleted the 4061-assert-keyword branch November 16, 2021 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate User-provided Keywords as Keywords
2 participants