c4-bot-10 commented 8 months ago

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

0xRobocop commented 8 months ago

QA

c4-pre-sort commented 8 months ago

0xRobocop marked the issue as insufficient quality report

3docSec commented 7 months ago

Point 1: invalid (inconsequential, and common practice), Point 2: QA

c4-judge commented 7 months ago

3docSec changed the severity to QA (Quality Assurance)