Ondo Finance - kaden's results

Lines of code

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

Vulnerability details

Impact

Admin's can't burn rOUSG of non-KYC'd users.

Proof of Concept

rOUSG.burn is documented as, "Admin burn function to burn rOUSG tokens from any account". However, it's not possible to burn shares from an account which has been removed from the KYC list or sanctioned.

As we can see, we execute _burnShares on the provided account:

_burnShares(_account, ousgSharesAmount);

_burnShares executes _beforeTokenTransfer with the _account as the from address. As we can see, if the from address is non-zero and doesn't pass KYC, we revert:

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

As a result, it's impossible to burn the shares of a non-KYC'd user, even as an admin.

Note that having KYC/sanctioned funds DoS is documented as a known issue, however this prevents even the admin from accessing those funds via a method which is intended to work on "any account".

Tools Used

• Manual review

Recommended Mitigation Steps

Create and call a specialized function to burn shares without validating KYC status which is only executable by the admin-only burn function.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Fails to comply to ERC20 spec in two ways as stated below.

Note that under EIP Compliance Checklist, it states: "We strive to keep rOUSG as ERC20 compliant as possible."

Proof of Concept

There are two ways in which the contract fails to adhere to the ERC20 spec:

1. It's possible for KYC'd users to transferFrom a 0 amount from any address even if they're not approved in any way

function transferFrom(
  address _sender,
  address _recipient,
  uint256 _amount
) public returns (bool) {
  uint256 currentAllowance = allowances[_sender][msg.sender];
  // @audit will succeed if _amount is 0
  require(currentAllowance >= _amount, "TRANSFER_AMOUNT_EXCEEDS_ALLOWANCE");

  _transfer(_sender, _recipient, _amount);
  _approve(_sender, msg.sender, currentAllowance - _amount);
  return true;
}

This is in defiance of the ERC20 spec where it states: "The function SHOULD throw unless the _from account has deliberately authorized the sender of the message via some mechanism."

2. rOUSG.burn incorrectly emits the Transfer event as a mint of rOUSG tokens when they're actually being burned

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
  );
  // @audit emits transfer from address(0) to sender, 
  //        should be from _account to address(0)
  emit Transfer(address(0), msg.sender, getROUSGByShares(ousgSharesAmount));
  emit TransferShares(_account, address(0), ousgSharesAmount);
}

This incorrect usage of the Transfer event results in indexing logic failing to properly track balances and totalSupply.

Tools Used

• Manual review

Recommended Mitigation Steps

1. Revert if the msg.sender in transferFrom has an allowance of 0
2. Fix the Transfer event to be from _account to address(0)

Assessed type

ERC20