Omniscia Bonq Audit

linked-address-list Code Style Findings

linked-address-list Code Style Findings

LIN-01C: Generic Typographic Mistakes

Description:

The referenced lines contain typographical mistakes or generic documentational errors (i.e. copy-paste) that should be corrected.

Example:

contracts/utils/linked-address-list.sol
29"79d3d ref must exist or be 0x0"

Recommendation:

We advise them to be corrected enhancing the legibility of the codebase.

Alleviation:

The Bonq Protocol team has partially fixed the issue by correcting the revert string while the documentation error still exists.

LIN-02C: Inconsistent Logical Checks

Description:

The linked check performed is based on an indirect assumption by the system instead of directly evaluating that the _element is equal to the _list._first data entry, similarly to the last element evaluation.

Example:

contracts/utils/linked-address-list.sol
90} else if (_element == _list._last) {
91  // simplified process for removing the last element
92  _list._last = _list._values[_element].prev;
93  _list._values[_list._last].next = _list._last;
94} else if (_element == _list._values[_element].prev) {
95  // simplified process for removing the first element
96  address next = _list._values[_element].next;
97  _list._values[next].prev = next;
98  _list._first = next;
99} else {

Recommendation:

We advise the code to be updated to use the same logical checks across its structure and the comparison to be performed with the list's _first data entry.

Alleviation:

The Bonq Protocol team has corrected the logical checks.

LIN-03C: Inefficient Utilization of Data Entries

Description:

The _list._last data entry is read three times redundantly instead of being stored to a local variable similarly to the removal process of the first element.

Example:

contracts/utils/linked-address-list.sol
90} else if (_element == _list._last) {
91  // simplified process for removing the last element
92  _list._last = _list._values[_element].prev;
93  _list._values[_list._last].next = _list._last;
94} else if (_element == _list._values[_element].prev) {
95  // simplified process for removing the first element
96  address next = _list._values[_element].next;
97  _list._values[next].prev = next;
98  _list._first = next;
99} else {

Recommendation:

We advise the codebase to be optimized as such by caching the result of _list._values[_element].prev to a local variable and consequently utilizing it for the assignments.

Alleviation:

The Bonq Protocol team has fixed the issue by optimizing the statements.

LIN-04C: Inefficient mapping Lookups

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

contracts/utils/linked-address-list.sol
31// the lement must not exist in order to be added
32if (_list._values[_element].prev == address(0x0)) {
33  // the list is empty
34  if (_list._last == address(0x0)) {
35    //                console.log("the list is empty, adding", _element, "as only element");
36    // if it is the first element in the list, it refers to itself to indicate this
37    _list._values[_element].prev = _element;
38    _list._values[_element].next = _element;
39    // the new element is now officially the first
40    _list._first = _element;
41    // the new element is now officially the last
42    _list._last = _element;
43  } else {
44    if (_before && (_reference == address(0x0) || _reference == _list._first)) {
45      //                    console.log("the list has %s elements. adding %s as first element", _list._size,  _element);
46      // the element should be added as the first element
47      address first = _list._first;
48      _list._values[first].prev = _element;
49      _list._values[_element].prev = _element;
50      _list._values[_element].next = first;
51      _list._first = _element;
52    } else if (!_before && (_reference == address(0x0) || _reference == _list._last)) {
53      //                    console.log("the list has %s elements. adding %s as last element", _list._size,  _element);
54      // the element should be added as the last element
55      address last = _list._last;
56      _list._values[last].next = _element;
57      _list._values[_element].prev = last;
58      _list._values[_element].next = _element;
59      _list._last = _element;
60    } else {
61      // the element should be inserted in between two elements
62      EntryLink memory ref = _list._values[_reference];
63      if (_before) {
64        _list._values[_element].prev = ref.prev;
65        _list._values[_element].next = _reference;
66        _list._values[_reference].prev = _element;
67        _list._values[ref.prev].next = _element;
68      } else {
69        _list._values[_element].prev = _reference;
70        _list._values[_element].next = ref.next;
71        _list._values[_reference].next = _element;
72        _list._values[ref.next].prev = _element;
73      }
74      //                    console.log("the list has '%s' elements. inserting '%s' ", _list._size, _element);
75    }
76  }
77  //            console.log("between %s and %s", _list._values[_element].prev, _list._values[_element].next);
78  _list._size = _list._size + 1;
79  return true;
80}

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

Alleviation:

The Bonq Protocol team has fixed the issue by storing a storage pointer to the struct.

Prev

external-price-feed.sol (EXT-C)

Next

mintable-token-owner.sol (MIT-C)

On this page