Signature Malleability in ERC721 Permit

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/base/PermitERC721.sol#L97-L110

Vulnerability details

Impact

Replay attacks are possible thanks to signature malleability

Proof of Concept

The ecrecover EVM opcode has a limitation that allows for non-unique signatures, which means that different signatures can produce the same output.

According to EIP-2, any transaction signatures with an s-value greater than secp256k1n/2 are considered invalid. However, validating such signatures by passing them to the _verify function is still possible.

To create a malleable signature, one can flip the s value from s to secp256k1n - s and also flip the v value (27 -> 28, 28 -> 27). Surprisingly, the resulting signature would still be valid.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/base/PermitERC721.sol#L97-L110

    address owner = ownerOf(tokenId_);
        require(spender_ != owner, "ERC721Permit: approval to current owner");

        if (Address.isContract(owner)) {
            // bytes4(keccak256("isValidSignature(bytes32,bytes)") == 0x1626ba7e
            require(
                IERC1271(owner).isValidSignature(digest, abi.encodePacked(r_, s_, v_)) == 0x1626ba7e,
                "ajna/nft-unauthorized"
            );
        } else {
            address recoveredAddress = ecrecover(digest, v_, r_, s_);
            require(recoveredAddress != address(0), "ajna/nft-invalid-signature");
            require(recoveredAddress == owner, "ajna/nft-unauthorized");
        }

Even though ERC721 permit could be out of scope, I've confirmed the issue with the sponsor and after discussing with the C4 team we agreed to fill it in: since the scope section in the audit repo isn't 100% clear here. So ultimately this will end up being a call the judge makes.

Tools Used

Manual Review
Discussion/confirmation with the Ajna Protocol team.

Recommended Mitigation Steps

To address this issue, OpenZeppelin has taken measures to handle malleable signatures. You can find more information on how they tackled this problem at this GitHub link: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L125.

Assessed type

Access Control

[c4-sponsor]

MikeHathaway marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as unsatisfactory:
Out of scope

[Picodes]

Unfortunately, the contract was clearly out of scope in the README.md of the contest. Although the finding is technically valid we can't include it in the price pool as audits are priced according to the number of LOC in scope. So it's up to the sponsor to reward this independantly.

[Picodes]

@MikeHathaway regarding #147, #141, #143 and this issue please see my comment above regarding out of scope findings.