Signature replay attack

[code423n4]

Lines of code

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

Vulnerability details

Impact

If a user is added to the registry via a signature signed by an address with the specified role and for some reason user is removed from KYC list, however the deadline > block.timestamp, the attacker can resubmit the same data and the transaction will success ,the KYC status will change to True

Proof of Concept

 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
    );
  }

In the code above, function addKYCAddressViaSignature() first check signature ,KYC status and deadline in lines 87-92, then build hash and perfrom check the singer.Finally set the KYC status of user to true. The check is not enough.If for some reason user is removed from KYC list and deladline has not expired, the attack can resubmit the same data and change the status to true.

Tools Used

Vscode

Recommended Mitigation Steps

Add nonce to measure

[c4-judge]

trust1995 marked the issue as duplicate of #187

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as partial-50