Omniscia rain protocol Audit

RainVM Manual Review Findings

RainVM Manual Review Findings

RVM-01M: Documentation Discrepancy of Opcode Consumption

Description:

The RainVM documentation states that scripts are read in RTL format and that the first byte of a particular script indicates the operation code to execute whilst the second byte indicates the argument of the operation. This is untrue as the actual implementation interprets the operation input as the first byte and the actual operation as the second byte.

Example:

contracts/vm/RainVM.sol
235assembly {
236 i_ := add(i_, 2)
237 let op_ := mload(add(sourceLocation_, i_))
238 opcode_ := byte(30, op_)
239 operand_ := byte(31, op_)
240}

Recommendation:

We advise the documentation to be revised as it can significantly affect applyOp implementors and cause them to misinterpret commands.

Alleviation:

The documentation was updated to properly reflect the way Rain VM scripts are consumed thereby alleviating this exhibit.

RVM-02M: Documentation Discrepancy of Operators

Description:

The linked lines highlight a documentation discrepancy in the RainVM whereby the operation 2 is mislabeled as ZIPMAP when in reality the code utilizes 3.

Example:

contracts/vm/RainVM.sol
50/// There are only 3 "core" opcodes for `RainVM`:
51/// - `0`: Skip self and optionally additional opcodes, `0 0` is a noop
52/// - `1`: Copy value from either `constants` or `arguments` at index `operand`
53/// to the top of the stack. High bit of `operand` is `0` for `constants` and
54/// `1` for `arguments`.
55/// - `2`: Zipmap takes N values from the stack, interprets each as an array of
56/// configurable length, then zips them into `arguments` and maps a source
57/// from `sources` over these. See `zipmap` for more details.
58///
59/// To do anything useful the contract that inherits `RainVM` needs to provide
60/// opcodes to build up an internal DSL. This may sound complex but it only
61/// requires mapping opcode integers to functions to call, and reading/writing
62/// values to the stack as input/output for these functions. Further, opcode
63/// packs are provided in rain that any inheriting contract can use as a normal
64/// solidity library. See `MathOps.sol` opcode pack and the
65/// `CalculatorTest.sol` test contract for an example of how to dispatch
66/// opcodes and handle the results in a wrapping contract.
67///
68/// RainVM natively has no concept of branching logic such as `if` or loops.
69/// An opcode pack could implement these similar to the core zipmap by lazily
70/// evaluating a source from `sources` based on some condition, etc. Instead
71/// some simpler, eagerly evaluated selection tools such as `min` and `max` in
72/// the `MathOps` opcode pack are provided. Future versions of `RainVM` MAY
73/// implement lazy `if` and other similar patterns.
74///
75/// The `eval` function is `view` because rain scripts are expected to compute
76/// results only without modifying any state. The contract wrapping the VM is
77/// free to mutate as usual. This model encourages exposing only read-only
78/// functionality to end-user deployers who provide scripts to a VM factory.
79/// Removing all writes remotes a lot of potential foot-guns for rain script
80/// authors and allows VM contract authors to reason more clearly about the
81/// input/output of the wrapping solidity code.
82///
83/// Internally `RainVM` makes heavy use of unchecked math and assembly logic
84/// as the opcode dispatch logic runs on a tight loop and so gas costs can ramp
85/// up very quickly. Implementing contracts and opcode packs SHOULD require
86/// that opcodes they receive do not exceed the codes they are expecting.
87abstract contract RainVM {
88 /// `0` is a skip as this is the fallback value for unset solidity bytes.
89 /// Any additional "whitespace" in rain scripts will be noops as `0 0` is
90 /// "skip self". The val can be used to skip additional opcodes but take
91 /// care to not underflow the source itself.
92 uint256 private constant OP_SKIP = 0;
93 /// `1` copies a value either off `constants` or `arguments` to the top of
94 /// the stack. The high bit of the operand specifies which, `0` for
95 /// `constants` and `1` for `arguments`.
96 uint256 private constant OP_VAL = 1;
97 /// Duplicates the top of the stack.
98 uint256 private constant OP_DUP = 2;
99 /// `2` takes N values off the stack, interprets them as an array then zips
100 /// and maps a source from `sources` over them. The source has access to
101 /// the original constants using `1 0` and to zipped arguments as `1 1`.
102 uint256 private constant OP_ZIPMAP = 3;
103 /// Number of provided opcodes for `RainVM`.
104 uint256 internal constant OPS_LENGTH = 4;

Recommendation:

We advise this to be corrected as it can lead to a significant discrepancy in integrators of the RainVM.

Alleviation:

The documentation was updated to properly associate each operation with its dedicated opcode numeric signal.

RVM-03M: Documentation Discrepancy of Zipmap Operand Unpacking

Description:

The documentation of the zipmap function indicates that 2 low bits are utilized as the index of the source to use from sources, however, 3 bits are actually used in the code. Additionally, 2 bits are actually used for the number of loop iterations instead of 3 bits.

Example:

contracts/vm/RainVM.sol
120/// The `operand_` for the zipmap opcode is split into 3 components:
121/// - 2 low bits: The index of the source to use from `sources`.
122/// - 3 middle bits: The size of the loop, where 0 is 1 iteration
123/// - 3 high bits: The number of vals to be zipped from the stack where 0
124/// is 1 value to be zipped.
125///
126/// This is a separate function to avoid blowing solidity compile stack.
127/// In the future it may be moved inline to `eval` for gas efficiency.
128///
129/// See https://en.wikipedia.org/wiki/Zipping_(computer_science)
130/// See https://en.wikipedia.org/wiki/Map_(higher-order_function)
131/// @param context_ Domain specific context the wrapping contract can
132/// provide to passthrough back to its own opcodes.
133/// @param state_ The execution state of the VM.
134/// @param operand_ The operand_ associated with this dispatch to zipmap.
135function zipmap(
136 bytes memory context_,
137 State memory state_,
138 uint256 operand_
139) internal view {
140 unchecked {
141 uint256 sourceIndex_;
142 uint256 stepSize_;
143 uint256 offset_;
144 uint256 valLength_;
145 // assembly here to shave some gas.
146 assembly {
147 // rightmost 3 bits are the index of the source to use from
148 // sources in `state_`.
149 sourceIndex_ := and(operand_, 0x07)
150 // bits 4 and 5 indicate size of the loop. Each 1 increment of
151 // the size halves the bits of the arguments to the zipmap.
152 // e.g. 256 `stepSize_` would copy all 256 bits of the uint256
153 // into args for the inner `eval`. A loop size of `1` would
154 // shift `stepSize_` by 1 (halving it) and meaning the uint256
155 // is `eval` as 2x 128 bit values (runs twice). A loop size of
156 // `2` would run 4 times as 64 bit values, and so on.
157 //
158 // Slither false positive here for the shift of constant `256`.
159 // slither-disable-next-line incorrect-shift
160 stepSize_ := shr(and(shr(3, operand_), 0x03), 256)
161 // `offset_` is used by the actual bit shifting operations and
162 // is precalculated here to save some gas as this is a hot
163 // performance path.
164 offset_ := sub(256, stepSize_)
165 // bits 5+ determine the number of vals to be zipped. At least
166 // one value must be provided so a `valLength_` of `0` is one
167 // value to loop over.
168 valLength_ := add(shr(5, operand_), 1)
169 }

Recommendation:

We advise this to be corrected as it can significantly impact integration efforts of the RainVM.

Alleviation:

The documentation was updated to properly indicate how the operand is meant to be split into 3 low - 2 mid - 3 high bits instead of 2 low - 3 mid - 3 high bits, alleviating this exhibit in full.

RVM-04M: Inexplicable Limitation of Jump Instructions

Description:

The "zero" command interpreted as the final else clause in the eval function's native opcode evaluation block contains a limitation in the numbers it can represent. In detail, the shift_ calculation it utilizes can represent numbers between [-64,63] for JUMPI-like skips and [0,127] for JUMP-like skips as it cycles to 0 for an operand_ value of 10000000. Furthermore, all jump operations that move backwards need to pop an element from the stack and are classified as JUMPI-like.

Example:

contracts/vm/RainVM.sol
288// if the high bit of the operand is nonzero then take
289// the top of the stack and if it is zero we do NOT
290// skip.
291// analogous to `JUMPI` in evm opcodes.
292// If high bit of the operand is zero then we always
293// skip.
294// analogous to `JUMP` in evm opcodes.
295// the operand is interpreted as a signed integer so
296// that we can skip forwards or backwards. Notable
297// difference between skip and jump from evm is that
298// skip moves a relative distance from the current
299// position and is known at compile time, while jump
300// moves to an absolute position read from the stack at
301// runtime. The relative simplicity of skip means we
302// can check for out of bounds behaviour at compile
303// time and each source can never goto a position in a
304// different source.
305
306// manually sign extend 1 bit.
307// normal signextend works on bytes not bits.
308int8 shift_ = int8(
309 uint8(operand_) & ((uint8(operand_) << 1) | 0x7F)
310);
311
312// if the high bit is 1...
313if (operand_ & 0x80 > 0) {
314 // take the top of the stack and only skip if it is
315 // nonzero.
316 state_.stackIndex--;
317 if (state_.stack[state_.stackIndex] == 0) {
318 continue;
319 }
320}
321if (shift_ != 0) {
322 if (shift_ < 0) {
323 // This is not particularly intuitive.
324 // Converting between int and uint and then
325 // moving `i_` back another 2 bytes to
326 // compensate for the addition of 2 bytes at
327 // the start of the next loop.
328 i_ -= uint8(~shift_ + 2) * 2;
329 } else {
330 i_ += uint8(shift_ * 2);
331 }
332}

Recommendation:

We advise the documentation around jump instruction handling to be expanded as in its current state it appears rather convoluted and with undocumented limitations.

Alleviation:

The Rain Protocol team stated that after internal discussions they have chosen to instead deprecate the skip opcode and plan to remove it entirely in an upcoming commit to the codebase. As a result, we consider this exhibit acknowledged.