fix: filter proposal give and want by sparseKeywords in zcf.reallocate #1076
Conversation
There was a problem hiding this comment.
If we have the bug I think we do, I don't understand why this seems to work at all.
At
you check offer safety by enumerating[keyword, amount] pairs of proposal.give, checking that the new allocation isGTE the allocation proposed to give. However, with this PR the new allocation may omit a keyword, in which casewill be
undefined.
Likewise for proposal.want. Once I understand what's going on here, I'll take a good look at rightsConservation.js.
I think offer safety is only supposed to check that want and give are satisfied. There is a separate check that currency is conserved. Anything outside those limits is up to the contract. The point of this bug fix is that if the contract decides to provide something that's not specified by anyone's proposals, it can add it and move it around as much as is useful without any offer having anything to say about it. |
There was a problem hiding this comment.
@katelynsills clarified. the newAllocations must still be as populated as before, so the danger I worried about is invalid.
LGTM
|
@Chris-Hibbert I misunderstood the PR as allowing more keyword positions to be absent, such as in the |
|
Added an edit clarifying what happens with the current allocation and the |
The bug was introduced in "filter proposal give and want by sparseKeywords" #1076. It allows a grifter contract (demonstrated here) to slip assets away from a victim one keyword at a time. The victim is left with neither their give or their want. The solution in facets is to reinstate the insistance that offer safety be checked against all keywords. We provide the feature that #1076 was aiming for by allowing reallocations that don't impact keywords in an offer's proposal.
The bug was introduced in "filter proposal give and want by sparseKeywords" #1076. It allows a grifter contract (demonstrated here) to slip assets away from a victim one keyword at a time. The victim is left with neither their give or their want. The solution in facets is to reinstate the insistance that offer safety be checked against all keywords. We provide the feature that #1076 was aiming for by allowing reallocations that don't impact keywords in an offer's proposal.
The bug was introduced in "filter proposal give and want by sparseKeywords" #1076. It allows a grifter contract (demonstrated here) to slip assets away from a victim one keyword at a time. The victim is left with neither their give or their want. The solution in facets is to reinstate the insistance that offer safety be checked against all keywords. We provide the feature that #1076 was aiming for by allowing reallocations that don't impact keywords in an offer's proposal.
* test: demonstrate an attack by a zoe contract to be fixed in facets The bug was introduced in "filter proposal give and want by sparseKeywords" #1076. It allows a grifter contract (demonstrated here) to slip assets away from a victim one keyword at a time. The victim is left with neither their give or their want. The solution in facets is to reinstate the insistance that offer safety be checked against all keywords. We provide the feature that #1076 was aiming for by allowing reallocations that don't impact keywords in an offer's proposal. * fix: Fix the Zoe offer-safety bug demonstrated by the grifter attack The fix is to insist that all offers retain offer safety across all keywords. The desired ability to rearrange values outside of the offer's keywords can be satisfied by explicitly allowing rearrangements that don't impact any of the proposal's keywords. This commit includes the bug fix but not a test that demonstrates the vulnerability. * test: correct the test to pass now that the bug is fixed. * fix: run offer safety check over the entire new allocation, not just the sparse keywords version. Co-authored-by: Kate Sills <[email protected]>
This PR fixes a bug (#1075) in
zcf.reallocatein which we were filtering and filling the current allocation bysparseKeywordsand asserting that thenewAllocationis filled forsparseKeywordsbut we were not filtering theproposal.wantorproposal.give, leading to valid partial reallocations being rejected for not meeting offer safety. See issue for more details.Adds two tests. One test ensures that an allocation of something that is not specified in the proposal is offer safe. Another test ensures that the
mintPayments.jscontract works even if the user hasproposal.wantandproposal.giveusing different keywords. The user gets a refund and the newly minted assets.Edit (5/8/2020, 4:55pm): Updated information about how we handle sparse keywords for the current allocation and the
newAllocation. We do not fill thenewAllocation, we only assert that all the sparseKeywords exist.Closes #1075