Fix incorrect check that point is not identity element

[stevieraykatz]

In FCL_Elliptic_ZZ.ecAff_isOnCurve where x and y are the coordinates of the public key we first validate that the point is not at infinity (0,0) and then verify that the points are not equivalent to the prime field modulus.

    function ecAff_isOnCurve(uint256 x, uint256 y) internal pure returns (bool) {
        if ( ((0 == x)&&( 0 == y)) || (x == p &&  y == p)) {
            return false;
        }

Today, this check fails to consider the case where x and y are some higher multiple of p. The remainder of the check whether the point is on the curve, as well as all subsequent curve calculations, are all done mod p, so these are equivalent representations of the (0,0) identity element but pass this critical, initial check. This means that an attacker can create a key pair such that for any single message with signature he can produce up to three additional public keys which will all be validated by ecdsa_verify.

This can be fixed by changing the logic to the following:

    function ecAff_isOnCurve(uint256 x, uint256 y) internal pure returns (bool) {
        if ((0 == x % p) && (0 == y % p)) {
            return false;
        }

The following PoC demonstrates the vulnerability:

pragma solidity ^0.8.0;

contract Test {

    //curve prime field modulus
    uint256 constant p = 0xFFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFF;
    //short weierstrass first coefficient
    uint256 constant a = 0xFFFFFFFF00000001000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFC;
    //short weierstrass second coefficient
    uint256 constant b = 0x5AC635D8AA3A93E7B3EBBD55769886BC651D06B0CC53B0F63BCE3C3E27D2604B;

    function testDuplicatePublicKey() public pure {
        uint256 x; uint256 y;

        (x,y) = (5, 31468013646237722594854082025316614106172411895747863909393730389177298123724);
        assert(ecAff_isOnCurve(x, y));                  //   correct   validation
        assert(ecAff_isOnCurve(x + p, y));              // incorrect   validation
        assert(ecAff_isOnCurveCORRECTED(x, y));         //   correct   validation
        assert(!ecAff_isOnCurveCORRECTED(x + p, y));    //   correct invalidation

        (x,y) = (6, 24739918328848417526480033809572464882617473122143770809273908485596236620747);
        assert(ecAff_isOnCurve(x, y));
        assert(ecAff_isOnCurve(x + p, y));
        assert(ecAff_isOnCurveCORRECTED(x, y));
        assert(!ecAff_isOnCurveCORRECTED(x + p, y));

        (x,y) = (8, 82784132184742902406611976374283817475915538652053173371854007222633983132083);
        assert(ecAff_isOnCurve(x, y));
        assert(ecAff_isOnCurve(x + p, y));
        assert(ecAff_isOnCurveCORRECTED(x, y));
        assert(!ecAff_isOnCurveCORRECTED(x + p, y));

        (x,y) = (9, 64265364456294082747112527522981594904960689488638802860996194286474266915479);
        assert(ecAff_isOnCurve(x, y));
        assert(ecAff_isOnCurve(x + p, y));
        assert(ecAff_isOnCurveCORRECTED(x, y));
        assert(!ecAff_isOnCurveCORRECTED(x + p, y));
    }

    function ecAff_isOnCurve(uint256 x, uint256 y) internal pure returns (bool) {
        if (((0 == x) && (0 == y)) || (x == p && y == p)) {
            return false;
        }
        unchecked {
            uint256 LHS = mulmod(y, y, p); // y^2
            uint256 RHS = addmod(mulmod(mulmod(x, x, p), x, p), mulmod(x, a, p), p); // x^3+ax
            RHS = addmod(RHS, b, p); // x^3 + a*x + b

            return LHS == RHS;
        }
    }

    function ecAff_isOnCurveCORRECTED(uint256 x, uint256 y) internal pure returns (bool) {
        // note >= instead of ==, and || instead of && (it wasn't || that was the problem, but the ==)
        if (((0 == x) && (0 == y)) || (x >= p || y >= p)) {
            return false;
        }
        unchecked {
            uint256 LHS = mulmod(y, y, p); // y^2
            uint256 RHS = addmod(mulmod(mulmod(x, x, p), x, p), mulmod(x, a, p), p); // x^3+ax
            RHS = addmod(RHS, b, p); // x^3 + a*x + b

            return LHS == RHS;
        }
    }
}

A similar issue was reported in this biconomy audit.

[rdubois-crypto]

This PR fixes unicity in the representation of a point. While it is not a vulnerability as such, but rather malleability, it is correct that unexpected uses of malleability can lead to disaster. Thanks for the update.

[Arash-Afshar]

I don't think this change is needed and instead a different one should be made. The current change protects against cases where x=2p, 3p, ... or y=2p, 3p, ... but this never happens since 2p, 3p, ... won't fit in the uint256 type and therefore the type system will protect against it. The only reason that you might want to do this change is if mod operation is more efficient than checking against 0 and p.

The description of the PR is correct in pointing out that the code accepts x and x+p < 2^256 (and similarly y and y+p < 2^256) as valid while it should only accept x and y. I believe the fix should be adding an additional check that validates if x < p && y < p. Putting it all together, I think the following change should be made:

if (x >= p || y >= p || ((x == 0) && (y == 0))) {
    return false;
}

@rdubois-crypto what do you think?

[wilsoncusack]

@stevieraykatz I think Arash's point here makes sense and we should probably just open a new PR. I do not think the x+p issue is mitigated by the modulo check.