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 ...