Ondo Finance contest - Bauer's results

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

1.No check mint fee is 0

CashManager.sol If the mintFee is not setted of 0 , the collateral.safeTransferFrom(msg.sender, feeRecipient, feesInCollateral); will transfer 0 amount to feeRecipient in the requestMint(), this will waste a lot of gas.

function requestMint(
    uint256 collateralAmountIn
  )
    external
    override
    updateEpoch
    nonReentrant
    whenNotPaused
    checkKYC(msg.sender)
  {
    if (collateralAmountIn < minimumDepositAmount) {
      revert MintRequestAmountTooSmall();
    }

    uint256 feesInCollateral = _getMintFees(collateralAmountIn);
    uint256 depositValueAfterFees = collateralAmountIn - feesInCollateral;

    _checkAndUpdateMintLimit(depositValueAfterFees);

    collateral.safeTransferFrom(msg.sender, feeRecipient, feesInCollateral);
    collateral.safeTransferFrom(
      msg.sender,
      assetRecipient,
      depositValueAfterFees
    );

    mintRequestsPerEpoch[currentEpoch][msg.sender] += depositValueAfterFees;

    emit MintRequested(
      msg.sender,
      currentEpoch,
      collateralAmountIn,
      depositValueAfterFees,
      feesInCollateral
    );
  }

Recommended Mitigation Steps：

if (feesInCollateral != 0)
    collateral.safeTransferFrom(msg.sender, feeRecipient, feesInCollateral);

2. whenPaused is useless in function call multiexcall()

CashManager.sol Three functions requestMint(),claimMint(),requestRedemption() have the not pause modifier. There is whenPaused modifier for function call multiexcall(), it means the multiexcall() can be trigger when the status is Paused, however at this point MANAGER_ADMIN stile not able to call function that is modified by not paused.

function multiexcall(
    ExCallData[] calldata exCallData
  )
    external
    payable
    override
    nonReentrant
    onlyRole(MANAGER_ADMIN)
    whenPaused
    returns (bytes[] memory results)
  {
    results = new bytes[](exCallData.length);
    for (uint256 i = 0; i < exCallData.length; ++i) {
      (bool success, bytes memory ret) = address(exCallData[i].target).call{
        value: exCallData[i].value
      }(exCallData[i].data);
      require(success, "Call Failed");
      results[i] = ret;
    }
  }

3. Use Two-Step Transfer Pattern for important contract change

Contracts implementing access control's, e.g. owner, should consider implementing a Two-Step Transfer pattern. Otherwise it's possible that the role mistakenly transfers ownership to the wrong address, resulting in a loss of the role.

CashManager.sol

  function setFeeRecipient(
    address _feeRecipient
  ) external override onlyRole(MANAGER_ADMIN) {
    address oldFeeRecipient = feeRecipient;
    feeRecipient = _feeRecipient;
    emit FeeRecipientSet(oldFeeRecipient, _feeRecipient);
  }

  function setAssetRecipient(
    address _assetRecipient
  ) external override onlyRole(MANAGER_ADMIN) {
    address oldAssetRecipient = assetRecipient;
    assetRecipient = _assetRecipient;
    emit AssetRecipientSet(oldAssetRecipient, _assetRecipient);
  }

  function setAssetSender(
    address newAssetSender
  ) external onlyRole(MANAGER_ADMIN) {
    address oldAssetSender = assetSender;
    assetSender = newAssetSender;
    emit AssetSenderSet(oldAssetSender, newAssetSender);
  }

  function setKYCRegistry(
    address _kycRegistry
  ) external override onlyRole(MANAGER_ADMIN) {
    _setKYCRegistry(_kycRegistry);
  }


OndoPriceOracle.sol

92: ```function setFTokenToCToken()```
106: ```function setOracle()```

OndoPriceOracleV2.sol
145: ```function setFTokenToOracleType()```
182: ```function setOracle()```
194: ```function setFTokenToCToken()```
233: ```function setFTokenToChainlinkOracle()```