namada

namada copied to clipboard

All unwraps should be replaced with pattern-match-fail, unless we have a specific reason to do otherwise in a certain case (but I'm not sure what that would be?). Pattern-match-fail is always safe and should prevent runtime panics, which in transaction execution when dependent on user input are always a bug (and a DoS vector).

unwraps/panics in replica code should be fine, except when validating user input. it's been a hot topic of debate in the interop team, whether we should straight up panic, or propagate errors through monadic binding (>>=). ultimately I'm in defense of panics when errors are unrecoverable. user input doesn't fall under this category, because we don't want byzantine clients to DoS Namada.

Besides the easy-to-stop unwraps, there are plenty other things that can panic - imho it's essential (and beneficial as the code evolves) to write tests covering the tx/vp execution, especially for the stuff that's not sandboxed in wasm

I did a sweep over v0.35.1, and couldn't find any major issues. Here are the notable findings:

• The tx_ibc_execute host function is using a reference counted object with Rc, unwrapping the inner value. The value is correctly unwrapped when refs=1, but care should be taken not to increment the ref counts again.
• There was an unused wasm memory exported to the guest, which was addressed in #3258.

The only major concern right now is moving away from our forked wasmer, which hasn't been updated in a while; the associated issue is #821. The forked wasmer has two important changes:

• ucontext_t is redefined on Apple silicon targets, to avoid performing unaligned reads when trap signals are raised.
• Various other alignment problems were also fixed (see commit 5783b1a5).

Out of these, at least the first one has been upstreamed. I'm not sure whether the second one has, too... Either way, we should attempt to upgrade to the 4.x series of wasmer.