Admin can’t burn tokens from either revoked KYC addresses or KYC addresses added to sanction list due to a check in _beforeTokenTransfer

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

The current implementation of the burn function in the rOUSG contract restricts the ability of the admin with the BURNER_ROLE to burn tokens from addresses that have either been removed from the KYC list (KYCRegistry::removeKYCAddresses) or added to the external chainalysis's sanction list. This is because the _beforeTokenTransfer function enforces KYC checks for all token transfer operations, including burning tokens. As a result, if an address is no longer KYC-approved or has been sanctioned, the burn function will fail to execute, preventing the admin from burning tokens as intended. This limitation can hinder essential operations, affecting the integrity and functionality of the protocol.

Proof of Concept

Let’s check the burn function which calls the internal _burnShares function (see line 632) :

contracts/ousg/rOUSG.sol:
618:   /**
619:    * @notice Admin burn function to burn rOUSG tokens from any account
620:    * @param _account The account to burn tokens from
621:    * @param _amount  The amount of rOUSG tokens to burn
622:    * @dev Transfers burned shares (OUSG) to `msg.sender`
623:    */
624:   function burn(
625:     address _account,
626:     uint256 _amount
627:   ) external onlyRole(BURNER_ROLE) {
628:     uint256 ousgSharesAmount = getSharesByROUSG(_amount);
629:     if (ousgSharesAmount < OUSG_TO_ROUSG_SHARES_MULTIPLIER)
630:       revert UnwrapTooSmall();
631: 
632:     _burnShares(_account, ousgSharesAmount);
633: 
634:     ousg.transfer(
635:       msg.sender,
636:       ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
637:     );
638:     emit Transfer(address(0), msg.sender, getROUSGByShares(ousgSharesAmount));
639:     emit TransferShares(_account, address(0), ousgSharesAmount);
640:   }

554:   function _burnShares(
555:     address _account,
556:     uint256 _sharesAmount
557:   ) internal whenNotPaused returns (uint256) {
558:     require(_account != address(0), "BURN_FROM_THE_ZERO_ADDRESS");
559: 
560:     _beforeTokenTransfer(_account, address(0), _sharesAmount);
561: 
562:     uint256 accountShares = shares[_account];
563:     require(_sharesAmount <= accountShares, "BURN_AMOUNT_EXCEEDS_BALANCE");
564: 
565:     totalShares -= _sharesAmount;
566: 
567:     shares[_account] = accountShares - _sharesAmount;
568: 
569:     return totalShares;
570:   }

As can be seen in _burnShares function it calls _beforeTokenTransfer in line 560 with the account that we want to burn tokens form. Here is the code of _beforeTokenTransfer:

contracts/ousg/rOUSG.sol:
586:   function _beforeTokenTransfer(
587:     address from,
588:     address to,
589:     uint256
590:   ) internal view {
591:     // Check constraints when `transferFrom` is called to facliitate
592:     // a transfer between two parties that are not `from` or `to`.
593:     if (from != msg.sender && to != msg.sender) {
594:       require(_getKYCStatus(msg.sender), "rOUSG: 'sender' address not KYC'd");
595:     }
596: 
597:     if (from != address(0)) {
598:       // If not minting
599:       require(_getKYCStatus(from), "rOUSG: 'from' address not KYC'd");
600:     }
601: 
602:     if (to != address(0)) {
603:       // If not burning
604:       require(_getKYCStatus(to), "rOUSG: 'to' address not KYC'd");
605:     }
606:   }

The _beforeTokenTransfer function enforces KYC checks for all token transfers. if we see code block in L597-L600 we can see the require statement that lock the ability of burning token from account that has false in its KYC status, since this function is also used when unwrap rOUSG it is very acceptable in its nature. However, the burn function should not incorporate these checks. As a result, the admin with the BURNER_ROLE cannot burn tokens from addresses that are no longer KYC-approved or have been added to the sanctions list.

If we dig deeper to see more about _getKYCStatus which is part of KYCRegistryClientUpgradeable contract:

contracts/kyc/KYCRegistryClient.sol:
64:   function _getKYCStatus(address account) internal view returns (bool) {
65:     return kycRegistry.getKYCStatus(kycRequirementGroup, account);
66:   }


contracts/kyc/KYCRegistry.sol:
119:   /**
120:    * @notice Get KYC status of `account` for the provided
121:    *         `kycRequirementGroup`. In order to return true, `account`'s state
122:    *         in this contract must be true and additionally pass a
123:    *         `sanctionsList` check.
124:    *
125:    * @param kycRequirementGroup KYC group to check KYC status for
126:    * @param account             Addresses to check KYC status for
127:    */
128:   function getKYCStatus(
129:     uint256 kycRequirementGroup,
130:     address account
131:   ) external view override returns (bool) {
132:     return
133:       kycState[kycRequirementGroup][account] &&
134:       !sanctionsList.isSanctioned(account);
135:   }

We can tell that the state of the KYC is managed by separate roles in a separate contract and can be revoked at any given moment either by:

1. Address has been added to sanctions list where it checked in KYCRegistry#L134: !sanctionsList.isSanctioned

2. Address is revoked when utilizing KYCRegistry::removeKYCAddresses by another role:

contracts/kyc/KYCRegistry.sol:
169:   /**
170:    * @notice Remove addresses from KYC list
171:    *
172:    * @param kycRequirementGroup KYC group associated with `addresses`
173:    * @param addresses           List of addresses to revoke KYC'd status
174:    */
175:   function removeKYCAddresses(
176:     uint256 kycRequirementGroup,
177:     address[] calldata addresses
178:   ) external onlyRole(kycGroupRoles[kycRequirementGroup]) {
179:     uint256 length = addresses.length;
180:     for (uint256 i = 0; i < length; i++) {
181:       kycState[kycRequirementGroup][addresses[i]] = false;
182:     }
183:     emit KYCAddressesRemoved(msg.sender, kycRequirementGroup, addresses);
184:   }

Thus, This inconsistency in enforcing KYC requirements hinders the admin's ability to perform critical token burning operations, impacting the overall functionality of the protocol.

Similar findings

• Admin can't burn tokens from blocklisted addresses because of a check in _beforeTokenTransfer  2023-09-ondo-findings#136

Tools Used

• Manual review

Recommended Mitigation Steps

Consider these analyzings before solving the issue

1. If the burn function will be used only by the admin; consider burning shares without checking the KYC status of the address we are burning from. But be caution here as we can not make changes to _burnShares function since it used in unwrap! perhaps another enforced burn shares function can do the job.

2. If the burn function would be use by another contracts as of a tokenomics features like the one used by ousgInstantManager and OUSG token, then add another burn function that would be used only by the admin and enforce the burn regardless of the the KYC status of the address we are burning from.

Assessed type

Error

[c4-pre-sort]

0xRobocop marked the issue as duplicate of #237

[c4-judge]

3docSec marked the issue as satisfactory