fix!: improve error message when a purse is overdrawn

[katelynsills]

This PR adds a new error message for the case in which a purse is overdrawn (a withdraw fails because the amount to be withdrawn was greater than the amount in the purse).

Without the PR, the error message is the raw error message from the Nat package. -65000 is negative. This is not a good error message.

Refs #3780
(We will be making changes to how fees are charged that may improve this further, so I don't want to close it yet).

[katelynsills]

@dckc and @Chris-Hibbert, whichever of you is free, I'm requesting a review as a Mark-substitute since he's out of office.

@kriskowal, can you check whether the error handling matches our best practices? If so, I'd like to record this in our wiki for the future.

[kriskowal]

RangeError includes stack overflow, for what it’s worth. There are no good idioms for Error classification in JavaScript. Subclassing Error is not fun. Duck-typing error objects is popular, like the err.code idiom in Node.js for carrying the exit code of a failed child process if that’s the cause of the error. Duck-typing by name is fragile since errors are a global shared type space. Having ERTP export a symboly-named property for errors that can be recognized here would probably be the least fragile solution.

[katelynsills]

Hmm, thinking about future improvements, would it make sense for the Nat package to export the symboly-named property for errors? That way any user of Nat could know that, for instance, this was a negative value error, and not a stack overflow.

If that makes sense, how would I go about making a symboly-named property for errors?

[erights]

Why symbol-named? What does it accomplish that isn't more easily accomplished by other means?

[kriskowal]

I suggest symbols only because Error objects are a global type namespace, especially for purposes of classifying them in a handler. Using unregistered symbols or weak side tables avoids collisions across purposes. Though, a well-coordinated global namespace (as in thenables) is sometimes nicer, it’s not guaranteed to compose well.

[dckc]

After reviewing "to q() or not to q()" for some time now, I'm not certain I know the rules, but using assert(...) here seems right.

[erights]

Glad to see this was successfully resolved --- not using q to quote amounts. @katelynsills comment

I think the amount to be withdrawn is more sensitive than pretty much anything else we have in vats at this point.

is correct. The actual amounts as well as the currentBalance are exactly the kind of private information that should not casually be revealed to callers.

I think I'm still trying to understand the exact attack that might happen to us in the short term that we are trying to prevent.

There is nothing more permanent than a temporary "solution". We're trying to write code that survives past the short term and remains secure in longer terms. We will face enough unanticipated problems during those transitions. Let's not add problems we knew enough to avoid.

An attack: Object A representing Alice calls object B representing Bob. In reaction to that abstract request from Alice, Bob tries to do a transaction involving assets that Bob legitimately has and knows about. But it fails and Bob doesn't catch the error. Alice now find out about amounts and balances that were none of her business.

Alice, realizing that she can provoke Bob to attempt a transaction that would fail, and that Bob may not catch the error to hide it from Alice, does this repeatedly to probe balances and amounts that are none of her business.

There are lots of opportunities of Alice to exploit these kinds of weaknesses. The alleged brand, or the literal portion of the error, or a dozen other things may help Alice infer info that's none of her business. I don't have a hard-and-fast rule for what things should and should not be quoted. But numerical balances are definitely over the line --- revealing too much private info. (As @dckc reminds us, E's and Monte's sealed exceptions actually solve the problem; but unfortunately by means not practical for JS.)

The terms "caller" and "stack" may make this leakage threat seem more local that it is. Errors are passed in fail calls and as reasons of rejected promises. We are talking about the error.message string. This string is conveyed between vats. Our currents public chains cannot keep secrets, but our solos can. Bob's secrets may be on a solo, accidentally leaking secrets to his client, Alice, that is elsewhere.

I can see an attack given a call stack, which is removed through SES, but I'm not immediately seeing an attack given the actual contents of the substitution values. It seems like the worst-case scenario would be if we put capabilities into the template contents. But do we ever want to do that?

We, the good guys, should never do that. However, as @dckc reminds us, we can only prevent a conspiring Chuck and Alice from communicating this way by superhuman vigilance during a security review of Bob's code. In any case, this is besides the point. In this thread: To q or not to q, that is the question. That question is only about leaking info, not capabilities.

Even something like putting an instance into the template contents wouldn't be very helpful, because it's an opaque object that has no identity. Is it intended that the errors are read by users with the user's petnames?

The intention is that errors convey only info to their callers, not identities. We are not in a position to enforce this within a vat but we do enforce it between vats.

However, your question will be very relevant to the causeway-level users, able to gather console-level unredacted output from several sites. The unredacted values sent by X to consoles are the actual substitution values, without prior stringification. All current consoles do locally stringify. But one can imagine a future logging system that is able to preserve the identities in these substitution values, and a UI that presents them filtered through a petname system. This is hugely beyond anything Causeway tried to do, but is interesting.