rdubois-crypto / FreshCryptoLib

Cryptographic Primitives for Blockchain Systems (solidity, cairo, C and rust)
MIT License
121 stars 22 forks source link

Add additional check to avoid infinite loop #61

Closed stevieraykatz closed 4 months ago

stevieraykatz commented 4 months ago

Issue: If we call ecZZ_mulmuladd_S_asm( -Gx, -Gy, 1, 1) then the current implementation hangs indefinitely in the for loop: https://github.com/rdubois-crypto/FreshCryptoLib/blob/027cb877dc78640b62e25fce63eccee102ac828d/solidity/src/FCL_elliptic.sol#L369-L372

This occurs because the method currently checks for u == 0 and v ==0 before determining if((H0==0)&&(H1==0)). With coordinates -Gx and -Gy, this check is true which results in both u and v being set to 0. Then the loop referenced above searches for the MSB of 0.

Fix: We can easily address this by re-checking if u == v == 0 and returning early inside this specific condition.

rdubois-crypto commented 4 months ago

This function assumes that the user prior checks that the point Q is on the curve (it will be done when pushing Q on the contract using it).

-gx, -gy doesn't belongs to the curve so this case shall not be flagged.

stevieraykatz commented 4 months ago

@rdubois-crypto apologies, I didn't explain the issue precisely.

The points in question aren't -Gx and -Gy but are actually the negation of G. Perhaps a better notation is: minusG_x and minusG_y which are indeed points on the curve.

In our testing, we were able to generate the coordinates in question by calling the following: (uint256 minusG_x, uint256 minusG_y) = _goEcdsaScalarMult(FCL.n-1);

This call leverages the go implementation of EcdsaScalarBaseMult.

Assuming we have this correct, then the subject issue is valid and we think this PR should be re-opened.

rdubois-crypto commented 4 months ago

Ok got it, it is actually the point (gx, -gy), it makes sense.