Ondo Finance - Arz's results

Lines of code

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

Vulnerability details

The ousgInstantManager is used to mint or redeem OUSG/rOUSG using USDC on a 1:1 ratio. To calculate the amount of OUSG we will mint or redeem we use the _getMintAmount() and _getRedemptionAmount() functions.

function _getMintAmount(
    uint256 usdcAmountIn,
    uint256 price
  ) internal view returns (uint256 ousgAmountOut) {
    uint256 amountE36 = _scaleUp(usdcAmountIn) * 1e18;
    ousgAmountOut = amountE36 / price;
  }

function _getRedemptionAmount(
    uint256 ousgAmountBurned,
    uint256 price
  ) internal view returns (uint256 usdcOwed) {
    uint256 amountE36 = ousgAmountBurned * price;
    usdcOwed = _scaleDown(amountE36 / 1e18);
  }

So for example if we want to mint and use 105 USDC worth $1 and the price of OUSG is 105 the calculation will be:

(105e6 * 1e12) * 1e18 / 105e18 = 1e18

However the problem here is that the USDC is always priced at 1$ because we are only using the amount of USDC being sent without the current price. This will create problems when the price of USDC depegs which happened for example last year.

Users will be able to use USDC to mint OUSG as if the price was 1$ even though the price can be lower allowing the users to profit from this. Or if the price goes above 1$, users will receive the amount of USDC when redeeming as if it was 1$ even though it is worth more.

Impact

If the price of USDC depegs the users will be able to mint OUSG using a discounted price or they will be able to receive USDC at a higher price when redeeming allowing them to profit from events like this.

Proof of Concept

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

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

function _getMintAmount(
    uint256 usdcAmountIn,
    uint256 price
  ) internal view returns (uint256 ousgAmountOut) {
    uint256 amountE36 = _scaleUp(usdcAmountIn) * 1e18;
    ousgAmountOut = amountE36 / price;
  }

function _getRedemptionAmount(
    uint256 ousgAmountBurned,
    uint256 price
  ) internal view returns (uint256 usdcOwed) {
    uint256 amountE36 = ousgAmountBurned * price;
    usdcOwed = _scaleDown(amountE36 / 1e18);
  }

As you can see here the amount of OUSG that the user will mint/redeem uses the amount of USDC sent which is always priced at 1$ and it doesnt check the current price.

Tools Used

Manual Review

Recommended Mitigation Steps

The easiest way to fix this is to get the USDC price from an oracle and revert if it is for example < 0.99$ or > 1.01$

Assessed type

Other

Lines of code

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

Vulnerability details

The rOUSG.sol contracts has a burner role which is able to burn rOUSG from any address. The burn() function will be used to seize rOUSG/OUSG when the address is not legally allowed to own it. The problem is that the user will likely be sanctioned before the rOUSG is seized to freeze the funds but because the _burnShares() function calls _beforeTokenTransfer(), the tx will revert because of the check of the from address in _beforeTokenTransfer() which will be the sanctioned address causing the check to revert.

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

Although this finding was submitted in the previous contest a big difference here is that the KYCRegistry uses a Chainalysis sanction oracle for the checks in _beforeTokenTransfer(). In the previous contest this could have been bypassed by the admin batch executing transactions but because we are now using the Chainalysis oracle we will not be able to simply remove the sanctioned address from the oracle, burn the tokens and add it again.

Impact

The admin wont be able to seize tokens from the sanctioned address because burning will always fail. The sanctioned address will be left with the tokens stuck in the wallet.

Proof of Concept

Add this to rOUSG.t.sol, as you can see burning from a sanctioned address will fail.

function testBurningROUSG() public dealAliceROUSG(1e18) {
    assertEq(rOUSGToken.balanceOf(alice), 100e18);

    vm.startPrank(OUSG_GUARDIAN);
    rOUSGToken.grantRole(rOUSGToken.BURNER_ROLE(), OUSG_GUARDIAN);

    //ALice gets added to the Chainalysis sanctions oracle
    _restrictOUSGUser(alice);

    vm.expectRevert("rOUSG: 'from' address not KYC'd");
    rOUSGToken.burn(alice,100e18);
    vm.stopPrank();
  }

Tools Used

Foundry

Recommended Mitigation Steps

Burn the tokens without the _beforeTokenTransfer() check

Assessed type

Token-Transfer