Ondo Finance contest - gzeon's results

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

Low

KYC can be removed while user still have fund in the protocol

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

  /**
   * @notice Remove addresses from KYC list
   *
   * @param kycRequirementGroup KYC group associated with `addresses`
   * @param addresses           List of addresses to revoke KYC'd status
   */
  function removeKYCAddresses(
    uint256 kycRequirementGroup,
    address[] calldata addresses
  ) external onlyRole(kycGroupRoles[kycRequirementGroup]) {
    uint256 length = addresses.length;
    for (uint256 i = 0; i < length; i++) {
      kycState[kycRequirementGroup][addresses[i]] = false;
    }
    emit KYCAddressesRemoved(msg.sender, kycRequirementGroup, addresses);
  }

Since function like claimMint and transfer require KYC, user fund might be stuck

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L241-L244

  function claimMint(
    address user,
    uint256 epochToClaim
  ) external override updateEpoch nonReentrant whenNotPaused checkKYC(user) {

Non-Critical

KYCRegistry does not check if address is already in the list

It is possible to add an already existing address or remove a non-existing address, emitting confusing events.

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

  /**
   * @notice Add addresses to KYC list for specified `kycRequirementGroup`
   *
   * @param kycRequirementGroup KYC group associated with `addresses`
   * @param addresses           List of addresses to grant KYC'd status
   */
  function addKYCAddresses(
    uint256 kycRequirementGroup,
    address[] calldata addresses
  ) external onlyRole(kycGroupRoles[kycRequirementGroup]) {
    uint256 length = addresses.length;
    for (uint256 i = 0; i < length; i++) {
      kycState[kycRequirementGroup][addresses[i]] = true;
    }
    emit KYCAddressesAdded(msg.sender, kycRequirementGroup, addresses);
  }

  /**
   * @notice Remove addresses from KYC list
   *
   * @param kycRequirementGroup KYC group associated with `addresses`
   * @param addresses           List of addresses to revoke KYC'd status
   */
  function removeKYCAddresses(
    uint256 kycRequirementGroup,
    address[] calldata addresses
  ) external onlyRole(kycGroupRoles[kycRequirementGroup]) {
    uint256 length = addresses.length;
    for (uint256 i = 0; i < length; i++) {
      kycState[kycRequirementGroup][addresses[i]] = false;
    }
    emit KYCAddressesRemoved(msg.sender, kycRequirementGroup, addresses);
  }

Manager can steal fund from the contract

This allow manager to execute arbitrary call, including transferring token in the contract.

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/CashManager.sol#L962-L964

      (bool success, bytes memory ret) = address(exCallData[i].target).call{
        value: exCallData[i].value
      }(exCallData[i].data);