witnet / elliptic-curve-solidity

Elliptic Curve arithmetic operations written in Solidity
MIT License
166 stars 40 forks source link

chore: update versions and fix code review errors #16

Closed mariocao closed 4 years ago

mariocao commented 4 years ago

Fix some errors from the core review in #15.

·-----------------------------------------------·---------------·-------|
|                Error Type                     |  Occurrences  | FIXED |
········································································|
| Costly loop                                   |           3   |    0  |
········································································|
| Compiler version not fixed                    |           8   |    2  |
········································································|
| Revert inside the if-operator                 |           3   |    3  |
········································································|
| Pure-functions should not read/change state   |           3   |    0  |
········································································|
| Prefer external to public visibility level    |          14   |   12  |
········································································|
| Use of assembly                               |           6   |    0  |
········································································|
| Implicit visibility level                     |           8   |    8  |
·-----------------------------------------------·---------------·-------·
| TOTAL                                         |          45   |   25  |
·-----------------------------------------------·---------------·-------·

Costly loop: NOT FIXED

This error relates to potentially lengthly loops that might incur in prohibitive gas consumption. The following will not be considered as bugs as they are intrinsic to elliptic curve operations and the gas cost has been estimated.

Compiler version not fixed: NOT FIXED

This issue arises from the fact that minor differences in the compiler versions are still allowed. This is considered an error since future compiler versions may handle certain language constructions in a way the developer did not foresee.

In the case of library repositories, it make sense to support the most widely used stable versions. In our case, it supports versions from 0.5.3 to 0.6.4, so that the error has been partially mitigated.

Revert inside the if-operator: FIXED

This implies that the tool detected a revert inside an if clause. The tool recommends the usage of require clauses. All will be fixed in the code.

Pure-functions should not read/change state: NOT FIXED

This implies that the tool detected pure functions that potentially write the state of the contract. These will not be fixed as the tool can not determine what assembly code does. In fact the errors specified are related to contracts that DO NOT HAVE STATE.

Prefer external to public visibility level: PARTIALLY FIXED

Whenever possible, the tool recommends the usage of external visibility instead of public, as the latter implies copying the arguments in the stack consuming more gas. Most of them will be fixed. The errors in Migrations.sol will not be fixed.

Use of assembly: NOT FIXED

The tool, whenever assembly code is detected, triggers an alarm. These will not be fixed as the assembly code is necessary.

Implicit visibility level: FIXED

This implies some of the state variables do not have visibility assigned, which by default assigns the public visibility.