Admin cannot burn rOUSG tokens from any account.

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/4791cd1946f834f27addbc162c1817e897de7bf9/contracts/ousg/rOUSG.sol#L618-L640

Vulnerability details

Impact

The functionality intended to allow administrators to burn rOUSG tokens from any account fails when the account in question is either not KYC compliant or is on a sanctions list. This limitation undermines the admin's ability to manage tokens in a way that may be necessary for regulatory compliance, token supply adjustment, or other critical administrative functions.

Proof of Concept

In rOUSG contract, the burn function allows admin to burn tokens from any specified account.

  /**
   * @notice Admin burn function to burn rOUSG tokens from any account
   * @param _account The account to burn tokens from
   * @param _amount  The amount of rOUSG tokens to burn
   * @dev Transfers burned shares (OUSG) to `msg.sender`
   */
  function burn(
    address _account,
    uint256 _amount
  ) external onlyRole(BURNER_ROLE) {
    uint256 ousgSharesAmount = getSharesByROUSG(_amount);
    if (ousgSharesAmount < OUSG_TO_ROUSG_SHARES_MULTIPLIER)
      revert UnwrapTooSmall();

    _burnShares(_account, ousgSharesAmount);

    ousg.transfer(
      msg.sender,
      ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
    );
    emit Transfer(address(0), msg.sender, getROUSGByShares(ousgSharesAmount));
    emit TransferShares(_account, address(0), ousgSharesAmount);
  }

https://github.com/code-423n4/2024-03-ondo-finance/blob/4791cd1946f834f27addbc162c1817e897de7bf9/contracts/ousg/rOUSG.sol#L618-L640

When the _burnShares function is called, it has a _beforeTokenTransfer hook that checks if the from address (the account which token is burnt) is KYCed or in sanction list. If the account is not KYCed and in sanction list, then the call would revert.

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

This setup inadvertently restricts the admin's ability to burn tokens, meaning break the functionality of the burn function

Tools Used

Manual

Recommended Mitigation Steps

In case of burning from admin, allow to burn without checking the KYC status of the account.

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256
  ) internal view {
+  bool isBurningByAdmin = (to == address(0) && hasRole(BURNER_ROLE, msg.sender));
+  if (!isBurningByAdmin) {
        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");
        }
      }
  }

Assessed type

Token-Transfer

[0xRobocop]

Consider QA.

DoS is temporary at worst, CONFIGURER_ROLE can change the KYCRegistry contract.

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[YD-Lee]

Consider QA.

DoS is temporary at worst, CONFIGURER_ROLE can change the KYCRegistry contract.

Hi @0xRobocop , I think this is valid. It's true the CONFIGURER_ROLE can change the user's kycState, but the role cannot change the sanctionsList, as the only provided function in ISanctionsList.sol is isSanctioned. So if user is sanctioned, admin cannot burn the user's rOUSG tokens.

interface ISanctionsList {
  function isSanctioned(address addr) external view returns (bool);
}

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/external/chainalysis/ISanctionsList.sol

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked issue #32 as primary and marked this issue as a duplicate of 32

[YD-Lee]

3docSec marked issue #32 as primary and marked this issue as a duplicate of 32

Hi @c4-judge @3docSec , I do not think this is a duplicate of #32. #32 only shows the admin cannot burn tokens after address is removed from KYC list. This only causes a temporary DoS at worst (like @0xRobocop said above), as admin can first add the address to KYC, burn the tokens, and then remove the address from KYC.

The point is admin cannot burn tokens from sanctioned addresses, instead of un-KYC addresses.

[3docSec]

@YD-Lee please refrain from commenting on issues outside of the post-judging-QA period. While you wait for this timeframe, you can review the backstage guidelines that include this and many more guidelines you should follow to keep your backstage access.

[thangtranth]

Hi @3docSec,

Can you please help review this? I think @YD-Lee is correct to point out that:

• un-KYCed addresses only lead to temporary DOS since admin can KYC and burn in a transaction.
• sanctioned addresses lead to permanent DOS.

So a valid issue has to point out both conditions or at least sanction condition.

Thank you.

[3docSec]

Interesting point.

In general, I would very much agree with you, but this specific case is a little different because the KYC and blocklists are checked together by the KYCRegistry.

So among the #32 group, each and every finding that suggests bypassing the KYC check implicitly covers the blocklist case, and root causes having the same reasonable fix are treated as the same finding on C4.