Omniscia rain protocol Audit
RainVM Manual Review Findings
RainVM Manual Review Findings
RVM-01M: Documentation Discrepancy of Opcode Consumption
Type | Severity | Location |
---|---|---|
Standard Conformity | RainVM.sol:L32, L33, L238, L239 |
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:
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
Type | Severity | Location |
---|---|---|
Standard Conformity | RainVM.sol:L50, L55, L98, L102, L104 |
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:
50/// There are only 3 "core" opcodes for `RainVM`:51/// - `0`: Skip self and optionally additional opcodes, `0 0` is a noop52/// - `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` and54/// `1` for `arguments`.55/// - `2`: Zipmap takes N values from the stack, interprets each as an array of56/// configurable length, then zips them into `arguments` and maps a source57/// from `sources` over these. See `zipmap` for more details.58///59/// To do anything useful the contract that inherits `RainVM` needs to provide60/// opcodes to build up an internal DSL. This may sound complex but it only61/// requires mapping opcode integers to functions to call, and reading/writing62/// values to the stack as input/output for these functions. Further, opcode63/// packs are provided in rain that any inheriting contract can use as a normal64/// solidity library. See `MathOps.sol` opcode pack and the65/// `CalculatorTest.sol` test contract for an example of how to dispatch66/// 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 lazily70/// evaluating a source from `sources` based on some condition, etc. Instead71/// some simpler, eagerly evaluated selection tools such as `min` and `max` in72/// the `MathOps` opcode pack are provided. Future versions of `RainVM` MAY73/// implement lazy `if` and other similar patterns.74///75/// The `eval` function is `view` because rain scripts are expected to compute76/// results only without modifying any state. The contract wrapping the VM is77/// free to mutate as usual. This model encourages exposing only read-only78/// 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 script80/// authors and allows VM contract authors to reason more clearly about the81/// input/output of the wrapping solidity code.82///83/// Internally `RainVM` makes heavy use of unchecked math and assembly logic84/// as the opcode dispatch logic runs on a tight loop and so gas costs can ramp85/// up very quickly. Implementing contracts and opcode packs SHOULD require86/// 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` is90 /// "skip self". The val can be used to skip additional opcodes but take91 /// 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 of94 /// the stack. The high bit of the operand specifies which, `0` for95 /// `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 zips100 /// and maps a source from `sources` over them. The source has access to101 /// 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
Type | Severity | Location |
---|---|---|
Standard Conformity | RainVM.sol:L120-L124, L147 |
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:
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 iteration123/// - 3 high bits: The number of vals to be zipped from the stack where 0124/// 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 can132/// 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 from148 // sources in `state_`.149 sourceIndex_ := and(operand_, 0x07)150 // bits 4 and 5 indicate size of the loop. Each 1 increment of151 // the size halves the bits of the arguments to the zipmap.152 // e.g. 256 `stepSize_` would copy all 256 bits of the uint256153 // into args for the inner `eval`. A loop size of `1` would154 // shift `stepSize_` by 1 (halving it) and meaning the uint256155 // is `eval` as 2x 128 bit values (runs twice). A loop size of156 // `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-shift160 stepSize_ := shr(and(shr(3, operand_), 0x03), 256)161 // `offset_` is used by the actual bit shifting operations and162 // is precalculated here to save some gas as this is a hot163 // performance path.164 offset_ := sub(256, stepSize_)165 // bits 5+ determine the number of vals to be zipped. At least166 // one value must be provided so a `valLength_` of `0` is one167 // 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
Type | Severity | Location |
---|---|---|
Logical Fault | RainVM.sol:L308-L310 |
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:
288// if the high bit of the operand is nonzero then take289// the top of the stack and if it is zero we do NOT290// skip.291// analogous to `JUMPI` in evm opcodes.292// If high bit of the operand is zero then we always293// skip.294// analogous to `JUMP` in evm opcodes.295// the operand is interpreted as a signed integer so296// that we can skip forwards or backwards. Notable297// difference between skip and jump from evm is that298// skip moves a relative distance from the current299// position and is known at compile time, while jump300// moves to an absolute position read from the stack at301// runtime. The relative simplicity of skip means we302// can check for out of bounds behaviour at compile303// time and each source can never goto a position in a304// 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 is315 // 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 then325 // moving `i_` back another 2 bytes to326 // compensate for the addition of 2 bytes at327 // 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.