Description

As you can notice, at the beginning of the function the value of x has increased by 1,
consequently it would produce an integer overflow and the result of said SQRT would be incorrect.

• Elliptic-curve-solidity\contracts\FastEcMul.sol:457-459 - Review indentation and
tabs.
• Elliptic-curve-solidity\contracts\FastEcMul.sol:39-41 - ab[1] = int256(- t[0]);
instead of ab[1] = int256(0 - t[0]); ab[3] = - t[1]; instead of ab[3] = 0 - t[1];
• Elliptic-curve-solidity\examples\Secp256k1.sol:0-31 - Despite being a pure function,
if a contract called it, the caller's privateKey could be published. A warning should be
displayed in the comment that reflects it to avoid problems for a user who implements it /
inherits it wrong.

Hi, I'm writing an EIP of non-transferable contracts. I'm wondering could you add an example of receiving an address from a private key. I would include references to the organization and the repository in my EIP.

Hi,

Thanks for your work. This is very helpful. Currently solidity version goes up to 0.8.x. Could we update the solidity version requirement?

• Modify contract to be a library with internal methods
• Update tests
• Update gas cost report
• Release new version to npm

Finding discovered during Red4Sec security audit.

Description

A bad implementation of the algorithm ecAdd was found. In this implementation, all cases and exceptions are not covered. This will produce undesired results, which can result in unhandled errors and/or denial of services in some circumstances.
 

Impact

During the audit, it has been observed that when making sums between two points in the ElipticCurve.sol library, the value of Y is not checked. Therefore, when the value of X1 and X2 is the same, the value of Y should be also verified, otherwise it could lead to an infinite point.

As we can see in the following examples taken from other known implementations (BouncyCastle, ANSSI-FR), the value of the Y is always checked in order to return an infinity point or not.

• https://github.com/microsoft/uprove-csharp-sdk/blob/c59911aec87d66bead02c6b33fb4587aaddffd98/ThirdParty/BouncyCastle/bc-trimmed/ECPoint.cs#L657-L661
• https://github.com/ANSSI-FR/libecc/blob/65ca8ad2a326a3e5b41dfc0a35a6ec16b41b8127/scripts/expand_libecc.py#L225-L226

Code References:

Mitigations

• Check the value of Y in order to detect whether to return an infinity point or not.
• Perform single tests to check all possible exceptions that can be produced by the tested functions.
• Perform all unit tests that carry out official versions of the implemented standard.

This issue aims to analyze (with the proper auditing tools) the potential vunerabilities that the library might have.

Code Review Result

Tool used: SmartCheck

Errors found

Summary:

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

Costly loop

1. File: contracts/FastEcMul.sol
   Lines: 428-428
   Severity: 1
   Pattern id: 17f23a

2. File: contracts/FastEcMul.sol
   Lines: 30-34
   Severity: 2
   Pattern id: 38f6c7

3. File: contracts/FastEcMul.sol
   Lines: 447-447
   Severity: 1
   Pattern id: 17f23a

Compiler version not fixed

1. File: examples/Secp256k1.sol
   Lines: 1-1
   Severity: 1

2. Pattern id: 23fc32
   File: test/TestEllipticCurve.sol
   Lines: 1-1
   Severity: 1
   Pattern id: 23fc32

3. File: benchmark/Secp256k1GasHelper.sol
   Lines: 1-1
   Severity: 1
   Pattern id: 23fc32

4. File: benchmark/EcGasHelper.sol
   Lines: 1-1
   Severity: 1
   Pattern id: 23fc32

5. File: contracts/FastEcMul.sol
   Lines: 1-1
   Severity: 1
   Pattern id: 23fc32

6. File: contracts/Migrations.sol
   Lines: 1-1
   Severity: 1
   Pattern id: 23fc32

7. File: contracts/Migrations.sol
   Lines: 1-1
   Severity: 1
   Pattern id: 23fc32

8. File: contracts/EllipticCurve.sol
   Lines: 1-1
   Severity: 1
   Pattern id: 23fc32

Revert inside the if-operator

All will be fixed in the code.

1. File: contracts/EllipticCurve.sol
   Lines: 44-45
   Severity: 1
   Pattern id: c56b12

2. File: contracts/EllipticCurve.sol
   Lines: 16-18
   Severity: 1
   Pattern id: c56b12

3. File: contracts/EllipticCurve.sol
   Lines: 302-306
   Severity: 1
   Pattern id: c56b12

Pure-functions should not read/change state

1. File: contracts/FastEcMul.sol
   Lines: 251-379
   Severity: 1
   Pattern id: 11314f

2. File: contracts/FastEcMul.sol
   Lines: 218-242
   Severity: 1
   Pattern id: 11314f

3. File: contracts/EllipticCurve.sol
   Lines: 39-60
   Severity: 1
   Pattern id: 11314f

Prefer external to public visibility level

1. File: examples/Secp256k1.sol
   Lines: 22-30
   Severity: 1
   Pattern id: 73ufc1

2. File: benchmark/Secp256k1GasHelper.sol
   Lines: 19-27
   Severity: 1
   Pattern id: 73ufc1

3. File: benchmark/EcGasHelper.sol
   Lines: 131-145
   Severity: 1
   Pattern id: 73ufc1

4. File: benchmark/EcGasHelper.sol
   Lines: 75-91
   Severity: 1
   Pattern id: 73ufc1

5. File: benchmark/EcGasHelper.sol
   Lines: 30-32
   Severity: 1
   Pattern id: 73ufc1

6. File: benchmark/EcGasHelper.sol
   Lines: 34-48
   Severity: 1
   Pattern id: 73ufc1

7. File: benchmark/EcGasHelper.sol
   Lines: 127-129
   Severity: 1
   Pattern id: 73ufc1

8. File: benchmark/EcGasHelper.sol
   Lines: 66-73
   Severity: 1
   Pattern id: 73ufc1

9. File: benchmark/EcGasHelper.sol
   Lines: 50-64
   Severity: 1
   Pattern id: 73ufc1

10. File: benchmark/EcGasHelper.sol
    Lines: 93-109
    Severity: 1
    Pattern id: 73ufc1

11. File: benchmark/EcGasHelper.sol
    Lines: 111-125
    Severity: 1
    Pattern id: 73ufc1

12. File: benchmark/EcGasHelper.sol
    Lines: 16-28
    Severity: 1
    Pattern id: 73ufc1

13. File: contracts/Migrations.sol
    Lines: 20-23
    Severity: 1
    Pattern id: 73ufc1

14. File: contracts/Migrations.sol
    Lines: 16-18
    Severity: 1
    Pattern id: 73ufc1

Use of assembly

1. File: contracts/FastEcMul.sol
   Lines: 354-356
   Severity: 1
   Pattern id: 109cd5

2. File: contracts/FastEcMul.sol
   Lines: 283-285
   Severity: 1
   Pattern id: 109cd5

3. File: contracts/FastEcMul.sol
   Lines: 224-239
   Severity: 1
   Pattern id: 109cd5

4. File: contracts/FastEcMul.sol
   Lines: 330-332
   Severity: 1
   Pattern id: 109cd5

5. File: contracts/FastEcMul.sol
   Lines: 306-308
   Severity: 1
   Pattern id: 109cd5

6. File: contracts/EllipticCurve.sol
   Lines: 49-57
   Severity: 1
   Pattern id: 109cd5

Implicit visibility level

1. File: examples/Secp256k1.sol
   Lines: 14-14
   Severity: 1
   Pattern id: b51ce0

2. File: examples/Secp256k1.sol
   Lines: 17-17
   Severity: 1
   Pattern id: b51ce0

3. File: examples/Secp256k1.sol
   Lines: 16-16
   Severity: 1
   Pattern id: b51ce0

4. File: examples/Secp256k1.sol
   Lines: 15-15
   Severity: 1
   Pattern id: b51ce0

5. File: examples/Secp256k1.sol
   Lines: 13-13
   Severity: 1
   Pattern id: b51ce0

6. File: benchmark/Secp256k1GasHelper.sol
   Lines: 16-16
   Severity: 1
   Pattern id: b51ce0

7. File: benchmark/Secp256k1GasHelper.sol
   Lines: 14-14
   Severity: 1
   Pattern id: b51ce0

8. File: benchmark/Secp256k1GasHelper.sol
   Lines: 17-17
   Severity: 1
   Pattern id: b51ce0

9. File: benchmark/Secp256k1GasHelper.sol
   Lines: 15-15
   Severity: 1
   Pattern id: b51ce0

10. File: benchmark/Secp256k1GasHelper.sol
    Lines: 13-13
    Severity: 1
    Pattern id: b51ce0

The goal is to perform simultaneous scalar multiplication in solidity in a generic manner, providing the needed constants through arguments

@mariocao commented on Mon Aug 12 2019

Integrate scalar decomposition support into the EllipticCurve contract:

• Implement scalar decomposition
• Tests

Hi!

Nice library to use, I really enjoy it, but could you update npm package with contracts, because there is still contract with >=0.5.3 <0.7.0 solidity version. It will be nice to use the most up to date lib

Thanks for your time!

The goal is to replicate the algorithm assessed and implemented in witnet/research#55 in solidity.

Red4Sec issue 16

Elliptic-curve-solidity\contracts\EllipticCurve.sol:299-301 - The logic specified in the comment does not match the implementation of the code.

Elliptic-curve-solidity\contracts\FastEcMul.sol:251-252 - variable _length not described in comments.