It is not possible to burn the tokens of a user who was removed from KYC registry.

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

In the rOUSG contract, the burn function calls the burnShares function, which internally calls the _beforeTokenTransfer function. Have a look at this check in the _beforeTokenTransfer function:

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

As the tokens are being burnt, the execution enters the above check as the from address is not 0. The problem lies in the require statement. It checks whether the KYC status of the from address is true. If false, the execution reverts.

This works well for users who have a positive KYC status. But, when the admin wants to burn the tokens from a user whose KYC status was removed, they wouldn't be able to do so. As the _getKYCStatus would output false for a user who was removed from the KYC registry and hence the function reverts.

Note that in the contest readme, the KYC related issue talks about this:

specifically when a user’s KYCRegistry or Sanction status changes in between different actions, leaving them at risk of their funds being locked. If someone gets sanctioned on the Chainalysis Sanctions Oracle or removed from Ondo Finance’s KYC Registry their funds are locked.

This is different from the issue being discussed here. The known issue talks about changing KYC status in between different actions. But, for this issue, KYC status need not change in between actions.

Proof of Concept

  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

The above check can be updated to have the following:

if (from != address(0) && to != address(0))

This would allow tokens to be burnt even from a user who does not have a positive KYC status.

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #237

[c4-judge]

3docSec marked the issue as satisfactory