Ondo Finance - Tychai0s's results

Institutional-Grade Finance, Now Onchain.

General Information

Platform: Code4rena

Start Date: 29/03/2024

Pot Size: $36,500 USDC

Total HM: 5

Participants: 72

Period: 5 days

Judge: 3docSec

Total Solo HM: 1

Id: 357

League: ETH

Ondo Finance

Findings Distribution

Researcher Performance

Rank: 20/72

Findings: 2

Award: $72.43

🌟 Selected for report: 0

🚀 Solo Findings: 0

Findings Information

Awards

64.1515 USDC - $64.15

Labels

bug
2 (Med Risk)
satisfactory
:robot:_26_group
duplicate-32

External Links

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/rOUSG.sol#L586 https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/rOUSG.sol#L624

Vulnerability details

Impact

The restriction imposed by the KYC check in the rOUSG.sol contract's _beforeTokenTransfer function affects the admin's ability to burn tokens for accounts that are not KYC-approved. This limitation could hinder administrative actions intended to comply with regulatory requirements or manage tokens in cases of fraud, sanctions, or other significant reasons.

Proof of Concept

In rOUSG.sol, the contract includes an admin function burn that allows tokens to be burned from any account by an authorized administrator (holding the BURNER_ROLE). However, this function's effectiveness is constrained by a KYC verification step within _beforeTokenTransfer. If an account has not been KYC-verified or is sanctioned, the function will revert, preventing the admin from burning tokens from that account.

function _burnShares(
    address _account,
    uint256 _sharesAmount
  ) internal whenNotPaused returns (uint256) {
    require(_account != address(0), "BURN_FROM_THE_ZERO_ADDRESS");

    _beforeTokenTransfer(_account, address(0), _sharesAmount);

    uint256 accountShares = shares[_account];
    require(_sharesAmount <= accountShares, "BURN_AMOUNT_EXCEEDS_BALANCE");

    totalShares -= _sharesAmount;

    shares[_account] = accountShares - _sharesAmount;

    return totalShares;
  }
// ...

function _beforeTokenTransfer(
    address from,
    address to,
    uint256
  ) internal view {
    // Check constraints when `transferFrom` is called to facliitate
    // a transfer between two parties that are not `from` or `to`.
    if (from != msg.sender && to != msg.sender) {
      require(_getKYCStatus(msg.sender), "rOUSG: 'sender' address not KYC'd");
    }

    if (from != address(0)) {
      // If not minting
      require(_getKYCStatus(from), "rOUSG: 'from' address not KYC'd");
    }

    if (to != address(0)) {
      // If not burning
      require(_getKYCStatus(to), "rOUSG: 'to' address not KYC'd");
    }
  }

Tools Used

Manual Review

Consider implementing a separate admin-controlled pathway that bypasses the KYC check for specific administrative actions like burning tokens. This pathway should include stringent checks and balances to ensure it is used appropriately, potentially including multi-signature requirements or additional logging for transparency and auditability. Such a mechanism would allow for more flexible management of tokens in exceptional circumstances while maintaining compliance and security standards.

Issue Type

Access Control

Assessed type

Access Control

#0 - c4-pre-sort

2024-04-04T05:10:42Z

0xRobocop marked the issue as duplicate of #237

#1 - c4-judge

2024-04-09T08:33:03Z

3docSec marked the issue as satisfactory

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/78779c30bebfd46e6f416b03066c55d587e8b30b/contracts/ousg/ousgInstantManager.sol#L567

Vulnerability details

Impact

This issue prevents the proper updating of the redeemFee parameter in the ousgInstantManager.sol contract. Specifically, the validation incorrectly checks the current redeemFee stored value instead of the new _redeemFee parameter. This could lead to a situation where the redeemFee remains inappropriately high or cannot be adjusted to a lower, intended value if it was previously set to 200 or above.

Proof of Concept

In ousgInstantManager.sol:567, the contract's setRedeemFee function is intended to allow the configuration of the redeem fee. However, due to an incorrect comparison (require(redeemFee < 200, "OUSGInstantManager::setRedeemFee: Fee too high");), the function may fail to update the fee even when a valid new fee is provided. This flaw arises because it validates against the current value of redeemFee instead of the new value _redeemFee.

require(redeemFee < 200, "OUSGInstantManager::setRedeemFee: Fee too high");
emit RedeemFeeSet(redeemFee, _redeemFee);
redeemFee = _redeemFee;

Tools Used

Manual Review

To address this vulnerability, the validation check within the setRedeemFee function should be corrected to compare the new _redeemFee parameter against the threshold. This adjustment ensures that updates to the redeemFee are evaluated based on the proposed new value, allowing for appropriate adjustments and maintaining the contract's operational integrity.

Issue Type

Invalid Validation

Assessed type

Invalid Validation

#0 - c4-pre-sort

2024-04-04T04:49:08Z

0xRobocop marked the issue as duplicate of #181

#1 - c4-judge

2024-04-09T14:51:56Z

3docSec changed the severity to QA (Quality Assurance)

#2 - c4-judge

2024-04-09T14:53:04Z

3docSec marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter