Ondo Finance - Krace's results

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L586-L606 https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/rOUSG.sol#L624-L640

Vulnerability details

Impact

The BURNER_ROLE cannot burn tokens if the target account has been removed from the KYC list.

Proof of Concept

When the BURNER_ROLE burns tokens of _account, it invokes _burnShares and then calls _beforeTokenTransfer to verify the KYC status of _account.

In accordance with a previous audit report, the BURNER_ROLE should have the capability to burn tokens of any account, even if the account is blacklisted or, in this case, not KYC verified. However, there is no mechanism that allows BURNER_ROLE to burn tokens of accounts that are removed from KYC list.

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

  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");
    }
// When from is not KYC, BURNER can not burn their tokens
    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");
    }
  }

POC

Add the test to forge-tests/ousg/rOUSG.t.sol and run it with:

forge test --fork-url $(grep -w ETHEREUM_RPC_URL .env | cut -d '=' -f2) --fork-block-number $(grep -w FORK_FROM_BLOCK_NUMBER_MAINNET .env | cut -d '=' -f2) --nmc ASSERT_FORK --match-test test_burn_with_NOKYC

diff --git a/forge-tests/ousg/rOUSG.t.sol b/forge-tests/ousg/rOUSG.t.sol
index 67faa15..b39b4ac 100644
--- a/forge-tests/ousg/rOUSG.t.sol
+++ b/forge-tests/ousg/rOUSG.t.sol
@@ -13,6 +13,7 @@ contract Test_rOUSG_ETH is OUSG_BasicDeployment {
     CashKYCSenderReceiver ousgProxied = CashKYCSenderReceiver(address(ousg));
     vm.startPrank(OUSG_GUARDIAN);
     ousgProxied.grantRole(ousgProxied.MINTER_ROLE(), OUSG_GUARDIAN);
+    ousgProxied.grantRole(ousgProxied.BURNER_ROLE(), OUSG_GUARDIAN);
     vm.stopPrank();

     // Sanity Asserts
@@ -26,6 +27,15 @@ contract Test_rOUSG_ETH is OUSG_BasicDeployment {
     assertTrue(registry.getKYCStatus(OUSG_KYC_REQUIREMENT_GROUP, alice));
   }

+  function test_burn_with_NOKYC() public dealAliceROUSG(1e18) {
+      vm.startPrank(OUSG_GUARDIAN);
+      _removeAddressFromKYC(OUSG_KYC_REQUIREMENT_GROUP, alice);
+      vm.stopPrank();
+
+      vm.startPrank(OUSG_GUARDIAN);
+      rOUSGToken.burn(alice, 1e18);
+      vm.stopPrank();
+  }
   /*//////////////////////////////////////////////////////////////
                         rOUSG Metadata Tests
   //////////////////////////////////////////////////////////////*/

Result:

Ran 1 test for forge-tests/ousg/rOUSG.t.sol:Test_rOUSG_ETH
[FAIL. Reason: revert: rOUSG: 'from' address not KYC'd] test_burn_with_NOKYC() (gas: 246678)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 11.44ms (1.15ms CPU time)

Ran 1 test suite in 1.12s (11.44ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in forge-tests/ousg/rOUSG.t.sol:Test_rOUSG_ETH
[FAIL. Reason: revert: rOUSG: 'from' address not KYC'd] test_burn_with_NOKYC() (gas: 246678)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Foundry

Recommended Mitigation Steps

Allow the BURNER to burn tokens without checking the KYC of from address.

diff --git a/contracts/ousg/rOUSG.sol b/contracts/ousg/rOUSG.sol
index 29d9112..6809a28 100644
--- a/contracts/ousg/rOUSG.sol
+++ b/contracts/ousg/rOUSG.sol
@@ -594,7 +594,7 @@ contract ROUSG is
       require(_getKYCStatus(msg.sender), "rOUSG: 'sender' address not KYC'd");
     }

-    if (from != address(0)) {
+    if (from != address(0) && !hasRole(BURNER_ROLE, msg.sender)) {
       // If not minting
       require(_getKYCStatus(from), "rOUSG: 'from' address not KYC'd");
     }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/be2e9ebca6fca460c5b0253970ab280701a15ca1/contracts/ousg/ousgInstantManager.sol#L278-L324

Vulnerability details

Impact

When minting OUSG via function mint, there is no slippage protection and the users may receive less amount of OUSG than they expected.

Proof of Concept

When users mint OUSG, the ousgPrice might change before the mint function executes, resulting in users receiving an unexpected amount of OUSG and incurring a loss of funds.

  function mint(
    uint256 usdcAmountIn
  )
    external
    override
    nonReentrant
    whenMintNotPaused
    returns (uint256 ousgAmountOut)
  {
    ousgAmountOut = _mint(usdcAmountIn, msg.sender);
    emit InstantMintOUSG(msg.sender, usdcAmountIn, ousgAmountOut);
  }

  function _mint(
    uint256 usdcAmountIn,
    address to
  ) internal returns (uint256 ousgAmountOut) {
    require(
      IERC20Metadata(address(usdc)).decimals() == 6,
      "OUSGInstantManager::_mint: USDC decimals must be 6"
    );
    require(
      usdcAmountIn >= minimumDepositAmount,
      "OUSGInstantManager::_mint: Deposit amount too small"
    );
    _checkAndUpdateInstantMintLimit(usdcAmountIn);
    if (address(investorBasedRateLimiter) != address(0)) {
      investorBasedRateLimiter.checkAndUpdateMintLimit(
        msg.sender,
        usdcAmountIn
      );
    }

    require(
      usdc.allowance(msg.sender, address(this)) >= usdcAmountIn,
      "OUSGInstantManager::_mint: Allowance must be given to OUSGInstantManager"
    );

    uint256 usdcfees = _getInstantMintFees(usdcAmountIn);
    uint256 usdcAmountAfterFee = usdcAmountIn - usdcfees;

    // Calculate the mint amount based on mint fees and usdc quantity
    uint256 ousgPrice = getOUSGPrice();
    ousgAmountOut = _getMintAmount(usdcAmountAfterFee, ousgPrice);

    require(
      ousgAmountOut > 0,
      "OUSGInstantManager::_mint: net mint amount can't be zero"
    );

    // Transfer USDC
    if (usdcfees > 0) {
      usdc.transferFrom(msg.sender, feeReceiver, usdcfees);
    }
    usdc.transferFrom(msg.sender, usdcReceiver, usdcAmountAfterFee);

    emit MintFeesDeducted(msg.sender, feeReceiver, usdcfees, usdcAmountIn);

    ousg.mint(to, ousgAmountOut);
  }

POC

Here's an example: Alice intends to mint OUSG with 100_000e6 USDC, expecting to receive 666666666666666666666 OUSG. However, if the price of OUSG increases before the mint transaction is executed, Alice may only receive 1/10 of the expected amount of OUSG.

Add the test to forge-tests/ousg/OUSGInstantManager/mint.t.sol and run it with:

forge test --fork-url $(grep -w ETHEREUM_RPC_URL .env | cut -d '=' -f2) --fork-block-number $(grep -w FORK_FROM_BLOCK_NUMBER_MAINNET .env | cut -d '=' -f2) --nmc ASSERT_FORK --match-test test_instant_mint__slippage -vvv

diff --git a/forge-tests/ousg/OUSGInstantManager/mint.t.sol b/forge-tests/ousg/OUSGInstantManager/mint.t.sol
index 2ec26e9..0d7ba02 100644
--- a/forge-tests/ousg/OUSGInstantManager/mint.t.sol
+++ b/forge-tests/ousg/OUSGInstantManager/mint.t.sol
@@ -106,6 +106,25 @@ contract Test_OUSGInstant_mint_ETH is OUSG_BasicDeployment, BUIDLHelper {
     assertEq(rOUSGToken.balanceOf(alice), 0);
   }

+  function test_instant_mint__slippage() public {
+    deal(address(USDC), alice, 100_000e6);
+
+    vm.startPrank(alice);
+    USDC.approve(address(ousgInstantManager), 100_000e6);
+    vm.stopPrank();
+
+    // The price goes up
+    oracleCheckHarnessOUSG.setPrice(1500e18);
+
+    // alice only get 66666666666666666666  rather than 666666666666666666666
+    // alice only get 1/10 OUSG than expected
+    vm.startPrank(alice);
+    ousgInstantManager.mint(100_000e6);
+    console.log("balance of alice ",ousg.balanceOf(alice));
+    vm.stopPrank();
+
+  }
+
   function test_instant_mint__fees() public {
     deal(address(USDC), alice, 100_000e6);

Tools Used

Foundry

Recommended Mitigation Steps

Add a slippage check to the mint function.

Assessed type

Context