onchain-id/solidity

Should issuer be either the Identity itself or the ClaimIssuer?

Web3Prof opened this issue · 0 comments

Claim has issuer property which should be an ONCHAINID address. Issuer should be an instance of ClaimIssuer or ONCHAINID with key purpose of 1 or 3, but since the isClaimValid is checking on the issuer's contract and inside the function it checks the keyHasPurpose . All instance of an Identity has the isClaimValid signature, so even if it's not a ClaimIssuer contract, it will return true because it has the correct signature and keyHasPurpose will always have a key purpose of one because the signer wallet address (that is retrieved from the getRecoveredAddress) is the initial key management of the issuer's identity.

if(_issuer != address(this)) {
            require(IClaimIssuer(_issuer).isClaimValid(IIdentity(address(this)), _topic, _signature, _data), "invalid claim"); // go to
        }

To solve this issue (to check that issuer is not some random ONCHAINID), I propose to add a function to check the key purpose on the claim receiver ONCHAINID isIssuerValid if the issuer is not self-attested, it must have the key purpose of 3 or check if address is an instance of ClaimIssuer' by adding a function of isClaimIssuer` in IClaimIssuer contract.

function isIssuerValid(
        IIdentity _identity,
        uint256 claimTopic,
        bytes memory sig,
        bytes memory data) public view returns(bool issuerValid){
        bytes32 dataHash = keccak256(abi.encode(_identity, claimTopic, data));
        // Use abi.encodePacked to concatenate the message prefix and the message to sign.
        bytes32 prefixedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", dataHash));

        // Recover address of data signer
        address recovered = getRecoveredAddress(sig, prefixedHash);

        // Take hash of recovered address
        bytes32 hashedAddr = keccak256(abi.encode(recovered));
        if (keyHasPurpose(hashedAddr, 3)) {
            return true;
        }
    }
// can only be called by an instance of ClaimIssuer, else will revert
    function isClaimIssuer() public pure override returns(bool){
        return true;
    }

I know there is a verifier contract that can check if issuer is valid or not, but I think giving additional checks during add claim reduce the amount of function called. Of course this could be adjusted depending on the business logic and this is just an idea proposal.

Minor bug: In the require part of checking the number of topics in Verifier.sol, it should be less than equal.

require(length <= 15 ...