`claimFighters()` is susceptible to signature malleability issue

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L206 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Verification.sol#L21-L40

Vulnerability details

Description

The issue lies in the require() call made in the in-scope FighterFarm.sol to Verification.sol out-of-scope. But the logic error retains in the in-scope function.

The elliptic curve (SECP256k1) used in ECDSA, as with all elliptic curves, is symmetrical about the x-axis. Therefore, for every (r,s,v), there exists another coordinate that returns the same valid result. This means that two signatures exist for one signer at any one point on the curve, allowing attackers to compute the second, valid signature without requiring the signer’s private key. Since the curve is reflected over the x-axis, there are two valid public keys for each value of r.

Impact

An attacker can exploit signature malleability by providing an equivalent signature that still verifies successfully. Specifically, if (r, s) is a valid signature for a message, then (r, -s) (where -s is the additive inverse of s modulo the order of the curve) is also a valid signature for the same message.

Proof of Concept

assembly {
            /*
            First 32 bytes stores the length of the signature

            add(sig, 32) = pointer of sig + 32
            effectively, skips first 32 bytes of signature

            mload(p) loads next 32 bytes starting at the memory address p into memory
            */

            // first 32 bytes, after the length prefix
            r := mload(add(signature, 32))
            // second 32 bytes
            s := mload(add(signature, 64))
            // final byte (first byte of the next 32 bytes)
            v := byte(0, mload(add(signature, 96)))
            //no checks for s value in lowerhalf order
            //v value to be 27

        }
        bytes memory prefix = "\x19Ethereum Signed Message:\n32";
        bytes32 prefixedHash = keccak256(abi.encodePacked(prefix, msgHash));
        return ecrecover(prefixedHash, v, r, s) == signer;
}

Tools Used

Manual Review

Recommended Mitigation Steps

When implementing ecrecover directly, the value of s should be restricted to only allow the lower levels.

This issue usually arises when ecrecover is used directly; therefore, OpenZeppelin’s ECDSA library (version 4.7.3 or greater) should be used.

The Cyfrin's article can be of great use to mitigate this issue.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #14

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Out of scope

[HickupHH3]

dup #867