Ondo Finance - leegh's results

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L618-L632 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L554-L560 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L586-L595 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/kyc/KYCRegistryClientUpgradeable.sol#L100-L102 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/kyc/KYCRegistry.sol#L128-L135

Vulnerability details

Impact

Admin can burn rOUSG tokens from any account via rOUSG.burn. But it's not true for sanctioned account addresses, which breaks the functionality of the function.

Proof of Concept

The target account's KYC status is first checked in function rOUSG.burn before burning, and the calling process is rOUSG.burn => rOUSG._burnShares => rOUSG._beforeTokenTransfer => KYCRegistryClientUpgradeable._getKYCStatus => KYCRegistry.getKYCStatus. The target account address is required to be KYCed and not sanctioned. Therefore, if the target account is a sanctioned address, the burn will revert and admin will not burn the tokens, which does not achieve the functionality of the function burn.

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);

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

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);

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

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:      }

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

100:    function _getKYCStatus(address account) internal view returns (bool) {
101:@>    return kycRegistry.getKYCStatus(kycRequirementGroup, account);
102:    }

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/kyc/KYCRegistryClientUpgradeable.sol#L100-L102

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:    }

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/kyc/KYCRegistry.sol#L128-L135

Tools Used

VSCode

Recommended Mitigation Steps

In the case of rOUSG.burn, we can ignore the _beforeTokenTransfer hook, or check the address is sanctioned outside of the _beforeTokenTransfer hook.

Assessed type

Invalid Validation