`rOUSG.sol` contract (token) isn't fully compilable with EIP20 standard

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L572-L606

Vulnerability details

Impact

• Non-Overridable _beforeTokenTransfer() Function in rOUSG.sol Limits EIP-20 Compliance and Extensibility

The rOUSG.sol contract's implementation of the _beforeTokenTransfer() function without the virtual keyword significantly impacts the contract's compliance with EIP-20 standards and its flexibility. EIP-20 (or ERC-20) is a widely adopted standard for Ethereum tokens, ensuring consistent behavior across tokens and compatibility with the broader Ethereum ecosystem. The inability to override _beforeTokenTransfer() restricts developers from introducing custom logic for token transfers, such as additional validations, transfer restrictions, or events that could enhance the token's security, usability, or integration capabilities. This limitation could deter adoption, complicate upgrades or extensions, and potentially expose the token to unforeseen risks by limiting the scope of governance or security measures that can be implemented in derived contracts.

Proof of Concept

The problem is that the rOUSG.sol#_beforeTokenTransfer() function is implemented without the virtual keyword, as described here in an auditor's notes which outlines best practices for ERC-20 token integrations, including the need for _beforeTokenTransfer and _afterTokenTransfer to be virtual and call super when overridden in derived contracts.

/**
 * @dev Hook that is called before any transfer of tokens. This includes
 * minting and burning.
 *
 * Calling conditions:
 *
 * - when `from` and `to` are both non-zero, `amount` of ``from``'s tokens
 * will be transferred to `to`.
 * - when `from` is zero, `amount` tokens will be minted for `to`.
 * - when `to` is zero, `amount` of ``from``'s tokens will be burned.
 * - `from` and `to` are never both zero.
 *
 * To learn more about hooks, head to xref:ROOT:extending-contracts.adoc#using-hooks[Using Hooks].
 */
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

Recommended Mitigation Steps

Update the _beforeTokenTransfer() function in the rOUSG contract by marking it as virtual. Ensure that derived contracts that override this function use the override keyword and properly call super._beforeTokenTransfer() to maintain the integrity of the hook's execution chain.

Assessed type

Context

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[3docSec]

rOUSG does not inherit ERC-20 functionality from any other contract and is not meant to be derived

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid