Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should issuer be either the Identity itself or the ClaimIssuer? #100

Open
Web3Prof opened this issue Apr 13, 2024 · 0 comments
Open

Should issuer be either the Identity itself or the ClaimIssuer? #100

Web3Prof opened this issue Apr 13, 2024 · 0 comments

Comments

@Web3Prof
Copy link

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 ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants
@Web3Prof and others