DBV1 Manual Review Findings

DBV-01M: Incorrect Addition of Value

Type	Severity	Location
Input Sanitization		DBV1.sol:L213

Description:

The DBV1::add(bytes32[],bytes32) function will add a value to the _valueList regardless of whether any keys were defined in the function's invocation.

Impact:

It is possible to corrupt the _valueList data entries by specifying a zero-length keys array and a value to the DBV1::add(bytes32[],bytes32) function.

Example:

Copy
197/// @inheritdoc IDB
198/// @custom:protected onlyOwner
199function add(bytes32[] memory keys, bytes32 value) public override onlyOwner {
200  Set storage valueSet = _values[value];
201
202  for (uint256 i = 0; i < keys.length; i++) {
203    bytes32 key = keys[i];
204    Set storage keySet = _keys[key];
205
206    keySet.exists = true;
207    valueSet.exists = true;
208
209    keySet.list.add(value);
210    valueSet.list.add(key);
211  }
212
213  _valueList.add(value);
214}

Recommendation:

We advise the DBV1::add(bytes32[],bytes32) code to either mandate that a non-zero length is present in the keys array or to execute the _valueList inclusion conditionally, either of which we consider an adequate resolution to this exhibit.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The 0xPhase team opted to apply the former of the two recommended courses of action, introducing a require check that mandates a non-zero length for the keys array and thus alleviates this exhibit.

DBV-02M: Unsafe Type Casting

Type	Severity	Location
Language Specific		DBV1.sol:L260

Description:

The DBV1::getAddressB32 function performs a casting operation of a bytes32 variable to a bytes20 variable unsafely, potentially truncating data points.

Impact:

It is presently possible to use any bytes32 variable stored in the contract as an address entry regardless of whether it contains more bits set than an address (i.e. regardless of whether it was meant for another purpose).

Example:

Copy
258/// @inheritdoc IDB
259function getAddressB32(bytes32 key) public view override returns (address) {
260  return address(bytes20(getValueB32(key)));
261}

Recommendation:

We advise it to evaluate that the bytes32 value yielded by DBV1::getValueB32 is at most 20 bytes in length, using either bitwise operators or numeric limits to achieve this.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The DBV1::getAddressB32 function properly validates the value it extracts via a bitwise operator in the latest implementation of the codebase, alleviating this exhibit.

DBV-03M: Discrepant Behaviour of NAND Operator

Type	Severity	Location
Logical Fault		DBV1.sol:L404

Description:

The NAND logical construct is meant to be satisfied if both mem and digested entries are not true, however, in the referenced instance only the case whereby the first entry (mem) is true and the second entry (digested) is false satisfies the circuit.

Impact:

The NAND operation code will fail to behave as expected in half of the evaluation instances, rendering the DBV1::_digest function insecure when dealing with NAND clauses.

Example:

Copy
399if (op == OpcodeType.AND) {
400  mem[j] = ((mem[j] > 0) && (digested[j] > 0)) ? 1 : 0;
401} else if (op == OpcodeType.OR) {
402  mem[j] = ((mem[j] > 0) || (digested[j] > 0)) ? 1 : 0;
403} else if (op == OpcodeType.NAND) {
404  mem[j] = ((mem[j] > 0) && !(digested[j] > 0)) ? 1 : 0;
405}

Recommendation:

We advise the first entry (mem) being false and the second entry (digested) being true to also satisfy the circuit by applying a logical negation (!) to the result of (mem[j] > 0) && (digested[j] > 0), properly implementing the NAND logical circuit's behaviour.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The behaviour of the NAND operator has been standardized to perform an actual NOT AND operation via the relevant logical operators, alleviating this exhibit.

DBV-04M: Incorrect Arithmetic Operator Methodology

Type	Severity	Location
Logical Fault		DBV1.sol:L345, L347, L349, L351

Description:

The DBV1::_digest function is expected to support multiple types of operators one category of which is arithmetic ones. In those operators, the code is expected to perform a calculation per element, however, the referenced calculations are performed with the currently active element which is always 0 rather than the previous one.

Impact:

The OpcodeType.ADD through to OpcodeType.DIV operators are presently broken in the current DBV1::_digest implementation, either misbehaving or being inoperable.

Example:

Copy
338for (uint256 j = 0; j < length; j++) {
339  if (i == 0) {
340    mem[j] = digested[j];
341    continue;
342  }
343
344  if (op == OpcodeType.ADD) {
345    mem[j] = mem[j] + digested[j];
346  } else if (op == OpcodeType.SUB) {
347    mem[j] = mem[j] - digested[j];
348  } else if (op == OpcodeType.MUL) {
349    mem[j] = mem[j] * digested[j];
350  } else if (op == OpcodeType.DIV) {
351    mem[j] = mem[j] / digested[j];
352  }
353}

Recommendation:

We advise the referenced instances to utilize j - 1 as an index to mem as otherwise they will greatly misbehave and at times be inoperable (i.e. in the subtraction).

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The elements are now properly consumed by utilizing the previous element and carrying it over with the relevant arithmetic operator and the digested value of the currently iterated element.

DBV-05M: Incorrect Logical Operator Methodology

Type	Severity	Location
Logical Fault		DBV1.sol:L400, L402, L404

Description:

The DBV1::_digest function is expected to support multiple types of operators one category of which is logical ones. In those operators, the code is expected to perform a logical combination (AND, OR, or NAND) per element pair, however, the referenced logical combinations are performed with the currently active element which is always 0 rather than the previous one.

Impact:

The OpcodeType.AND through to OpcodeType.NAND operators are presently broken in the current DBV1::_digest implementation, yielding significantly discrepant results.

Example:

Copy
388for (uint256 j = 0; j < length; j++) {
389  if (i == 0) {
390    if (op == OpcodeType.NAND) {
391      mem[j] = (digested[j] > 0 ? 0 : 1);
392    } else {
393      mem[j] = (digested[j] > 0 ? 1 : 0);
394    }
395
396    continue;
397  }
398
399  if (op == OpcodeType.AND) {
400    mem[j] = ((mem[j] > 0) && (digested[j] > 0)) ? 1 : 0;
401  } else if (op == OpcodeType.OR) {
402    mem[j] = ((mem[j] > 0) || (digested[j] > 0)) ? 1 : 0;
403  } else if (op == OpcodeType.NAND) {
404    mem[j] = ((mem[j] > 0) && !(digested[j] > 0)) ? 1 : 0;
405  }
406}

Recommendation:

We advise the referenced instances to utilize j - 1 as an index to mem as otherwise they will greatly misbehave.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The elements are now properly consumed by utilizing the previous element and carrying it over with the relevant logical operator and the digested value of the currently iterated element.

DBV-06M: Incorrect Removal of Value

Type	Severity	Location
Logical Fault		DBV1.sol:L99

Description:

The DBV1::removePair function will remove the specified input value from the global _valueList regardless of whether the value is still referenced by another key.

Impact:

A value will be removed from the _valueList incorrectly if it is pointed to by multiple keys, rendering the database's data structure corrupt.

Example:

Copy
89/// @inheritdoc IDB
90/// @custom:protected onlyOwner
91function removePair(bytes32 key, bytes32 value) external override onlyOwner {
92  if (!hasPair(key, value)) return;
93
94  Set storage keySet = _keys[key];
95  Set storage valueSet = _values[value];
96
97  keySet.list.remove(value);
98  valueSet.list.remove(key);
99  _valueList.remove(value);
100
101  if (keySet.list.length() == 0) {
102    valueSet.exists = false;
103  }
104
105  if (valueSet.list.length() == 0) {
106    valueSet.exists = false;
107  }
108}

Recommendation:

We advise the value to be removed from the _valueList solely when valueSet.exists is set to false, meaning that it is no longer pointed to by any key.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The code was updated per our recommendation, removing the value from the _valueList during a correct system state and thus alleviating this exhibit.