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

setMathHelpers leaks history-dependent full ordering of remotables #4261

Closed
erights opened this issue Jan 9, 2022 · 0 comments · Fixed by #4305
Closed

setMathHelpers leaks history-dependent full ordering of remotables #4261

erights opened this issue Jan 9, 2022 · 0 comments · Fixed by #4305
Assignees
Labels
audit-zestival Vulnerability assessment of ERTP + Zoe bug Something isn't working ERTP package: ERTP Zoe package: Zoe
Milestone

Comments

@erights
Copy link
Member

erights commented Jan 9, 2022

POTENTIAL SECURITY BUG

Currently, because the setMathHelpers and the amountMath built on it are static, we have a dilemma leading to an information leak. The algorithms in https://github.com/Agoric/agoric-sdk/blob/master/packages/store/src/keys/merge-set-operators.js benefit tremendously by being able to fully order a set of elements, not just rank[1] sort them. However, remotables a) can be elements of sets, and b) cannot be objectively ordered. As an expedient temporary measure, at

https://github.com/Agoric/agoric-sdk/blob/5ac65ecdfeed42be99f9316172b2799f70e37562/packages/ERTP/src/mathHelpers/setMathHelpers.js#L21-27

we capture a history-dependent full order into a top level variable, which is therefore mutable static state than can be used as a communications channel. Although the comparator itself is not exposed, the order of the resulting arrays is visible, which is equivalent in power.

The proper solution to this is a breaking change where the setMathHelper, and an amountMath using it are multiply instantiated, and each ERTP issuer locally makes its own instance. Note that there is no distributed coordination needed in any case, as the setMathHelper coerce function will always first reorder based on its own full comparator.

This communications channel is not an immediate concern, since we're currently running on a public chain that is fully transparent (no confidentiality, can keep no secrets) anyway. All the info that's leaked is already public info, and so no one can rely on it being secret anyway.

  • [1] A full order is one in which x <= y and y <= x iff x == y. A rank order is one in which x and y can be different but still be in the same equivalence class as far as the order is concerned. IOW, x and y are tied for the same rank (e.g., tied for second place). Array.prototype.sort is a stable sort, where the whole concept of a stable sort is only meaningful for a rank order. For a full order, there's no observable difference between a stable sort and a non-stable sort. The objective compareRank function defines only a rank order that does not leak information. But such orderings are too weak to enable the algorithms in merge-set-operators.js .
@erights erights added bug Something isn't working ERTP package: ERTP Zoe package: Zoe audit-zestival Vulnerability assessment of ERTP + Zoe labels Jan 9, 2022
@erights erights self-assigned this Jan 18, 2022
@Tartuffo Tartuffo added the MN-1 label Jan 20, 2022
@mergify mergify bot closed this as completed in #4305 Jan 29, 2022
@Tartuffo Tartuffo added this to the Mainnet 1 milestone Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-zestival Vulnerability assessment of ERTP + Zoe bug Something isn't working ERTP package: ERTP Zoe package: Zoe
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants