Ondo Finance
Findings & Analysis Report
2024-04-24
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [M-01] Integration issue in
ousgInstantManagerwithBUIDLifminUSTokensis set by blackrock - [M-02] Inadequate handling of
BUIDLredemption limit inOUSGinstant manager - [M-03] Users can lose access to funds due to minimum withdrawal limits
- [M-04] The
BURNERcannot burn tokens from accounts not KYC verified due to the check in_beforeTokenTransfer.
- [M-01] Integration issue in
-
Low Risk and Non-Critical Issues
- Summary
- L-01 No oracle price staleness checks
- L-02 User blocked by Circle cannot redeem
- L-03
usdcReceiveris immutable - L-04
ousgInstantManager::mint/redeemlacks slippage parameter - L-05 Lack of safe transfer wrapper
- NC-01 Unnecessary checks
- NC-02
usdcfeesdoesn’t followcamelCase - NC-03 Inconsistent
address(0)checks - NC-04 Erroneous math in documentation
- NC-05 Misspelled parameter
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the Ondo Finance smart contract system written in Solidity. The audit took place between March 29 — April 3, 2024.
Wardens
74 Wardens contributed reports to Ondo Finance:
- asui
- Breeje
- immeas
- Arz
- HChang26
- Limbooo
- Bigsam
- carrotsmuggler
- radev_sw
- dvrkzy
- 0xmystery
- 0xCiphky
- ZanyBonzy
- popeye
- b0g0
- Krace
- ni8mare
- ast3ros
- Shubham
- kartik_giri_47538
- kaden
- Tychai0s
- leegh
- yotov721
- SpicyMeatball
- 0xDemon
- m4ttm
- JC
- SAQ
- zabihullahazadzoi
- MaslarovK
- cheatc0d3
- 0xMosh
- OxTenma
- Stormreckson
- samuraii77
- 0xGreyWolf
- Honour
- niser93
- Dots
- bareli
- grearlake
- pkqs90
- slvDev
- baz1ka
- 0xJaeger
- 0xabhay
- 0xlemon
- jaydhales
- 0xAkira
- pfapostol
- btk
- arnie
- Omik
- Abdessamed
- EaglesSecurity (julian_avantgarde and kane-goldmisth)
- oualidpro
- Aymen0909
- 0xweb3boy
- DanielArmstrong
- FastChecker
- IceBear
- igbinosuneric
- caglankaan
- VAD37
- Aamir
- nonn_ac
- DarkTower (0xrex and haxatron)
- Tigerfrake
- dd0x7e8
- albahaca
- K42
This audit was judged by 3docSec.
Final report assembled by thebrittfactor.
Summary
The C4 analysis yielded an aggregated total of 5 unique vulnerabilities. Of these vulnerabilities, 1 received a risk rating in the category of HIGH severity and 4 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 64 reports detailing issues with a risk rating of LOW severity or non-critical.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Ondo Finance repository, and is composed of 3 smart contracts written in the Solidity programming language and includes 851 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (1)
[H-01] OUSGInstantManager will allow excessive OUSG token minting during USDC depeg event
Submitted by Breeje, also found by Arz, HChang26, and immeas
Any user can use mint function in ousgInstantManager contract to mint OUSG tokens by providing USDC token. It calls internal function _mint where the main logic resides.
function _mint(uint256 usdcAmountIn, address to) internal returns (uint256 ousgAmountOut) {
// SNIP: Validation
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");
// SNIP: Transfering USDC
ousg.mint(to, ousgAmountOut);
}Two important points to understand
OUSG Price Stability:
The contract depends on the OUSG price obtained from an oracle, which is heavily constrained (as per Readme) to ensure stability.
OUSGPrice - TheOUSGprice tracks an off chain portfolio of cash equivalents and treasury bills, price changes are heavily constrained in theOUSGOracle, which uses the change in the price of SHV to set the allowableOUSGprice in between updates. We are aware that the SHV price could differ from theOUSGportfolio, so any findings related to this price discrepancy is out of scope. Also, scenarios where theOUSGprice increases by many orders of magnitudes are not realistic and consequently not considered valid.
As per RWAOracleRateCheck Oracle, constraints includes:
OUSGprice updates restricted to once every23 hours.- Price deviations limited to a maximum of
1%.
function setPrice(int256 newPrice) external onlyRole(SETTER_ROLE) {
if (newPrice <= 0) {
revert InvalidPrice();
}
@-> if (block.timestamp - priceTimestamp < MIN_PRICE_UPDATE_WINDOW) {
revert PriceUpdateWindowViolation();
}
@-> if (_getPriceChangeBps(rwaPrice, newPrice) > MAX_CHANGE_DIFF_BPS) {
revert DeltaDifferenceConstraintViolation();
}
// Set new price
int256 oldPrice = rwaPrice;
rwaPrice = newPrice;
priceTimestamp = block.timestamp;
emit RWAPriceSet(oldPrice, newPrice, block.timestamp);
}These constraints ensure relative stability of the OUSG price.
Calculation Assumptions:
The calculation of the amount of OUSG tokens to mint assumes a fixed conversion rate of 1 USDC = 1 USD.
Key point: The _getMintAmount function calculates the OUSG amount based on the provided USDC amount and the OUSG price obtained from the oracle (by just upscaling and dividing).
function _getMintAmount(
uint256 usdcAmountIn,
uint256 price
) internal view returns (uint256 ousgAmountOut) {
uint256 amountE36 = _scaleUp(usdcAmountIn) * 1e18;
ousgAmountOut = amountE36 / price;
}Here, there are no validation checks implemented regarding the current USDC price.
Scenario of the issue
Consider Alice’s attempt to mint OUSG tokens by providing 100,000 USDC, assuming no minting fees and OUSG price of 105e18 USD. The calculation yields: 100_000e36 / 105e18 which is approximately 95_000e18 or 95_000 OUSG tokens for the 100_000 USDC provided.
However, in the event of a USDC depeg, where USDC’s value deviates from 1 USD:
- The contract’s calculation logic remains unchanged.
- Despite the depeg, the
OUSGprice remains fairly constant (maximum 1% deviation allowed in 23 hours).
This scenario leads to Alice getting close to 95_000 OUSG tokens again for 100_000 USDC provided. But this time, 100_000 USDC can be worth as low as 87_000 USD if we take recent depeg event in March 2023, where USDC price went as low as 87 cents (reference).
This way, contract will allow users to mint excessive OUSG tokens during the depeg event.
Impact
Minting of excessive token in case of USDC depeg.
Tools Used
VS Code
Recommended Mitigation Steps
Ideally, there needs to be an additional Oracle to check current price of USDC and take its price into the consideration when calculation OUSG tokens to mint.
Assessed type
Context
3docSec (judge) increased severity to High and commented:
Upgraded as High because there is risk of value extraction from the protocol under conditions that can be monitored by an attacker.
cameronclifton (Ondo) confirmed, but disagreed with severity and commented:
After further review, we will be mitigating this by adding a Chainlink
USDC/USDoracle to theOUSGInstantManagercontract. If the price is lower than what we are comfortable with, all mints and redemptions will be blocked. While we think it is unlikely that we won’t be able to convertUSDC->USD1:1 in our backend systems, we decided to do this out of extreme caution.
Note: For full discussion, see here.
Medium Risk Findings (4)
[M-01] Integration issue in ousgInstantManager with BUIDL if minUSTokens is set by blackrock
Submitted by asui
Integration issues with BUIDL, in the case blackrock decides to set a minimum amount of BUIDL tokens that should be held by its holders.
Proof of Concept
Blackrock implements a minUSTokens; variable where it sets a minimum amount to be held by the whitelisted addresses at all times. This check is done at every transfer. Currently, this is set to 0; but this could be set by blackrock at anytime.
// get min us tokens
function getMinUSTokens() public view override returns (uint256) {
return minUSTokens;
}
// set min us tokens
function setMinUSTokens(uint256 _value) public override onlyTransferAgentOrAbove {
emit DSComplianceUIntRuleSet("minUSTokens", minUSTokens, _value);
minUSTokens = _value;
}This is the code from the BUIDL token's contracts/compliance/ComplianceConfigurationService.Sol where the admin could set values for minUSTokens.
Also the line 238 in the contracts/compliance/Compliance/ServiceRegulated.sol is called when transferring token.
if (
_args.fromInvestorBalance > _args.value &&
_args.fromInvestorBalance - _args.value < IDSComplianceConfigurationService(_services[COMPLIANCE_CONFIGURATION_SERVICE]).getMinUSTokens()
) {
return (51, AMOUNT_OF_TOKENS_UNDER_MIN);
}This essentially checks that the sender has at least the minimum amount of tokens after the transfer.
The problem is the ousgInstantManager doesn’t require that it always has this much amount of BUIDL. When redeeming, suppose it has 300k BUIDL and the minimum is say 10k BUIDL and an investor in Ondo wants to redeem 300k BUIDL tokens. It would still revert even though the contract has it, which could unexpectedly revert; violating one the main functionalities for Ondo (i.e. the instant redeem).
Code
Create a new test file and paste this code below:
//SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;
import {Test, console} from "forge-std/Test.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
interface IBUILDPause {
function pause() external;
function isPaused() external returns(bool);
}
interface IBUiLDRedeemer {
function redeem(uint256 amount) external;
}
// 0x1e695A689CF29c8fE0AF6848A957e3f84B61Fe69
contract testBUILD is Test {
// holders of BUILD tokens; just for test
address holder1 = 0x72Be8C14B7564f7a61ba2f6B7E50D18DC1D4B63D;
address holder2 = 0xEd71aa0dA4fdBA512FfA398fcFf9db8C49A5Cf72;
address holder3 = 0xdc77C1D2A1dC61A31BE81e4840368DffEFAC3add;
address holder4 = 0x1e695A689CF29c8fE0AF6848A957e3f84B61Fe69;
address holder5 = 0xBc2cb4bF5510A1cc06863C96196a2361C8462525;
address holder6 = 0xc02Ac677e58e40b66f100be3a721bA944807C2D7;
address holder7 = 0x12c0de58D3b720024324d5B216DDFE8B29adB0b4;
address holder8 = 0xb3c62fbe3E797502A978f418582ee92a5F327C23;
address holder9 = 0x568430C66F9A256f609Ac07190d70c2c2573E065;
// we get the owner form etherscan
address ownerOfBUILD = 0xe01605f6b6dC593b7d2917F4a0940db2A625b09e;
address build = 0x7712c34205737192402172409a8F7ccef8aA2AEc; // build token address
IERC20 BUILD;
uint256 MAINNET_FORK;
function setUp() external {
MAINNET_FORK = vm.createFork("https://eth-mainnet.g.alchemy.com/v2/IrK2bvsF-q028QswCasD1dQqxV8nqGMs");
vm.selectFork(MAINNET_FORK);
BUILD = IERC20(build);
}
function testBUILDHolderTransfer() public {
address sender = holder1;
address to = holder9;
uint amountToSend = 90000000e6;
uint totalBalance = BUILD.balanceOf(sender);
vm.startPrank(sender); // random 5 million holder
BUILD.transfer(to, amountToSend); // transfer 1 million to alice
console.log(totalBalance);
console.log(BUILD.balanceOf(sender));
console.log(BUILD.balanceOf(to));
}
function testMinTokensUS() external { //0x1dc378568cefD4596C5F9f9A14256D8250b56369
COMPLIANCE compliance = COMPLIANCE(0x1dc378568cefD4596C5F9f9A14256D8250b56369); // compliance configuration service
console.log(compliance.getMinUSTokens());
console.log(compliance.getUSLockPeriod());
vm.startPrank(0xe01605f6b6dC593b7d2917F4a0940db2A625b09e); // owner address form etherscan
compliance.setMinUSTokens(10000000e6);
console.log(compliance.getMinUSTokens());
vm.stopPrank();
address sender = holder1;
address to = holder9;
uint amountToSend = 90000000e6;
vm.startPrank(sender);
BUILD.transfer(to, amountToSend);
uint totalBalance = BUILD.balanceOf(sender);
console.log(totalBalance);
console.log(BUILD.balanceOf(sender));
console.log(BUILD.balanceOf(to));
}Run forge test --mt testBUILDHolderTransfer which will pass but run forge test --mt testMinTokensUS -vvv, i.e. with the same amount after owner sets minUSTokens to 10 million, it will revert.
Note: 10 million is a very large amount and not realistic, it is just to show for the test because the holder 1 has more than 90 million BUIDL. I just set to 10 million to show it will revert. The real value set could be significantly lower than this.
Here in our example, when holder1 tries to transfer the code checks and notice that after transfer the holder will have less than the minimum, i.e. 10 million, so it reverts.
Tools Used
Foundry
Recommended Mitigation Steps
Import IDSComplianceConfigurationService or create an interface just for the getMinUSTokens() function and consider replacing the require statement in the _redeemBUIDL function with:
function _redeemBUIDL(uint256 buidlAmountToRedeem) internal {
require(
buidl.balanceOf(address(this)) - IDSComplianceConfigurationService(0x1dc378568cefD4596C5F9f9A14256D8250b56369).getMinUSTokens >= minBUIDLRedeemAmount,
"OUSGInstantManager::_redeemBUIDL: Insufficient BUIDL balance"
);The contract will never try to redeem more than its minimum allowed to hold and appropriately reverts with our error message: ”OUSGInstantManager::_redeemBUIDL: Insufficient BUIDL balance”
We get the address 0x1dc378568cefD4596C5F9f9A14256D8250b56369 of the complianceConfigurationService proxy by querying the BUIDL contract in etherscan using the function no.27 getDSService with 256 as the argument.
This minimum amount required may not be set currently but could be set by the admin in the future. So, implementing it now should be more compatible with BUIDL, even if in the future blackrock decides to set it.
Assessed type
Invalid Validation
cameronclifton (Ondo) acknowledged, but disagreed with severity and commented:
We will not mitigate this in the smart contract code. We plan to work with the BUIDL team to better understand the conditions in which
minUSTokenswill be set.
Note: For full discussion, see here.
[M-02] Inadequate handling of BUIDL redemption limit in OUSG instant manager
Submitted by Limbooo, also found by Bigsam
Impact
The OUSG Instant Redemption Manager contract contains an oversight in its redeem function, specifically in the handling of BUIDL redemption limits. This oversight can potentially lead to failed redemption attempts when the redemption balance exceeds the BUIDL balance held by the manager contract while it has a right amount if its concatenated with USDC amount left by another redemption process. The impact of this issue is significant as it affects the usability of the redemption feature and can result in user frustration and loss of trust in the system.
Proof of Concept
The following POC demonstrates the issue. Use it as part of forge-tests/ousg/OUSGInstantManager/redeem.t.sol file.
Run using this command:
npm run test-forge -- --match-test test_POC_redeem_fail_when_alice_redeemtion_balance_is_over_manager_BUIDL_balance function test_POC_redeem_fail_when_alice_redeemtion_balance_is_over_manager_BUIDL_balance()
public
setupSecuritize(500_000e6, 500_000e6)
{
uint256 aliceOUSGRedeemAmount = 1667e18;
uint256 aliceUSDCAmount = 250_100e6;
uint256 bobOUSGRedeemAmount = 1666e18;
uint256 bobUSDCAmount = 249_900e6;
// Mint OUSG tokens for Alice and Bob
vm.prank(address(ousgInstantManager));
ousg.mint(alice, aliceOUSGRedeemAmount);
vm.prank(address(ousgInstantManager));
ousg.mint(bob, bobOUSGRedeemAmount);
// Bob redeems OUSG tokens successfully
vm.startPrank(bob);
ousg.approve(address(ousgInstantManager), (bobOUSGRedeemAmount));
ousgInstantManager.redeem(bobOUSGRedeemAmount);
vm.stopPrank();
// Alice attempts to redeem OUSG tokens, but the redemption fails due to insufficient BUIDL balance
vm.startPrank(alice);
ousg.approve(address(ousgInstantManager), (aliceOUSGRedeemAmount));
vm.expectRevert('Not enough tokens');
ousgInstantManager.redeem(aliceOUSGRedeemAmount);
vm.stopPrank();
assertEq(USDC.balanceOf(bob), bobUSDCAmount);
assertEq(
BUIDL.balanceOf(address(ousgInstantManager)) + USDC.balanceOf(address(ousgInstantManager)),
aliceUSDCAmount
);
// However if Alice try to reddem an amount that will be in usdc amount <= minBUIDLRedeemAmount in ousgInstantManager it will success
vm.startPrank(alice);
ousgInstantManager.redeem(aliceOUSGRedeemAmount - 10e18);
vm.stopPrank();
// Tokens remaining in Alice's balance after successful redemption
assertEq(ousg.balanceOf(alice), 10e18);
}Tools Used
Foundry
Recommended Mitigation Steps
To address this issue, the OUSG Instant Redemption Manager contract should implement a mechanism to ensure that redemption requests do not exceed the available BUIDL balance held by the manager contract. This can be achieved by incorporating proper checks and balances in the redemption process, such as verifying the BUIDL balance before processing redemption requests and adjusting the redemption amount accordingly. Additionally, consider an redeem implementation that concatenate the balance of remaining USDC amount with the BUIDL redeemed balance if the corresponding USDC amount or redeem amount of OUSG is more than minBUIDLRedeemAmount.
Assessed type
Error
cameronclifton (Ondo) confirmed and commented:
Due to changing requirements, the contract will now concatenate the USDC amount with
BUIDLwhen performing redemptions. (This should mitigate this already known issue).
Note: For full discussion, see here.
[M-03] Users can lose access to funds due to minimum withdrawal limits
Submitted by carrotsmuggler, also found by Breeje, 0xmystery, radev_sw, dvrkzy, and 0xCiphky
The InstantManager contract restricts deposits and withdrawals to certain minimum amounts. Users can deposit a minimum of 100k USDC tokens, and withdraw a minimum of 50k USDC tokens.
The issue is that the minimum withdrawal limit can lead to users losing access to part of their funds. Say a user deposits 100k USDC tokens and then later withdraws 60k USDC tokens. Now, the user only has 40k USDC worth holdings in their account, and cannot withdraw the full amount. This is because it falls below the minimum withdrawal limit of 50k USDC tokens. The user is now stuck with 40k USDC tokens in their account, and cannot withdraw them.
The only option the user has is to deposit 100k USDC more, and then withdraw the whole 140k USDC amount. This will incur fees on the extra 100k USDC the user brings as well. Thus this is a Medium severity issue.
Proof of Concept
The scenario can be recreated in the following steps:
- User ALICE deposits 100k
USDCtokens. - User ALICE withdraws 60k
USDCtokens. - User ALICE tries to withdraw 40k
USDCtokens. The contract reverts, as the amount is below the minimum withdrawal limit of 50kUSDCtokens.
Recommended Mitigation Steps
Allow users to remove all their funds from the contract even if it is below the minimum limit. Since the protocol now uses a more liquid system such as the BUIDL token, this should be possible and should not affect the protocol’s functioning.
I acknowledge this behavior is a design decision. However, I would keep this as a valid Medium for an audit report:
- There is an availability impact for users, in a condition that they did not necessarily have to purposely create for themselves.
- Users can decide to still withdraw for a loss in fees “for minting more to redeem all”.
- The report highlights what I find to be a very reasonable mitigation - which could be the behavior users reasonably expect:
Allow users to remove all their funds from the contract even if it is below the minimum limit.
This mitigation seems feasible and difficult to exploit for systematic, abusive bypasses of
minimumRedemptionAmount, because bothOUSGandrOUSGhave a KYC requirement on token holders.
cameronclifton (Ondo) disputed and commented:
We will not be removing minimum redemption requirement from the smart contract as there are other means in which users can redeem OUSG or rOUSG tokens from Ondo Finance.
Note: For full discussion, see here.
[M-04] The BURNER cannot burn tokens from accounts not KYC verified due to the check in _beforeTokenTransfer.
Submitted by Krace, also found by ni8mare, Limbooo, leegh, ast3ros, radev_sw, Shubham, kartik_giri_47538, Arz, kaden, Tychai0s, yotov721, SpicyMeatball, ZanyBonzy, dvrkzy, and 0xDemon
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_NOKYCdiff --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 succeededTools 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
The reasons why I opted to keep this as Medium is:
- The possibility of changing a contract implementation (in this case the registry) to a new implementation (that is not in scope) is not something that is generally accepted as a severity mitigation
- The same finding was already judged as a valid Medium in a previous audit with a different scope (rUSDY), that was not explicitly marked as a known issue in the README
cameronclifton (Ondo) disputed and commented:
We will not be addressing this as we have a safe workaround for this exact scenario.
Note: For full discussion, see here.
Low Risk and Non-Critical Issues
For this audit, 64 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by immeas received the top score from the judge.
The following wardens also submitted reports: popeye, asui, Breeje, b0g0, ZanyBonzy, m4ttm, JC, SAQ, zabihullahazadzoi, MaslarovK, HChang26, cheatc0d3, ni8mare, 0xMosh, OxTenma, Stormreckson, samuraii77, 0xGreyWolf, Honour, niser93, Dots, ast3ros, bareli, radev_sw, grearlake, pkqs90, slvDev, baz1ka, 0xJaeger, 0xabhay, 0xlemon, jaydhales, 0xAkira, Shubham, pfapostol, 0xmystery, btk, arnie, Omik, carrotsmuggler, kaden, Abdessamed, Tychai0s, EaglesSecurity, kartik_giri_47538, oualidpro, Aymen0909, 0xweb3boy, DanielArmstrong, FastChecker, IceBear, igbinosuneric, caglankaan, VAD37, Aamir, nonn_ac, DarkTower, Tigerfrake, 0xCiphky, dd0x7e8, albahaca, K42, and Krace.
Summary
| ID | Title |
|---|---|
| [L-01] | No oracle price staleness checks |
| [L-02] | User blocked by Circle cannot redeem |
| [L-03] | usdcReceiver is immutable |
| [L-04] | ousgInstantManager::mint/redeem lacks slippage parameter |
| [L-05] | Lack of safe transfer wrapper |
| [NC-01] | Unnecessary checks |
| [NC-02] | usdcfees doesn’t follow camelCase |
| [NC-03] | Inconsistent address(0) checks |
| [NC-04] | Erroneous math in documentation |
| [NC-05] | Misspelled parameter |
[L-01] No oracle price staleness checks
Both ousgInstantManager and rOUSG uses an oracle to determine the price of OUSG.
File: contracts/ousg/rOUSG.sol
378: function getOUSGPrice() public view returns (uint256 price) {
379: (price, ) = oracle.getPriceData();
380: }Very similar in ousgInstantManager::getOUSGPrice but with a “lowest” price check.
Here only the first parameter, price is used. However, the second parameter returned is the priceTimestamp, which is the timestamp at which the price was updated. If this is old it can lead to incorrect OUSG prices used for rOUSG or instant minting/redeeming.
Recommendation
Consider adding a check to confirm the price used isn’t stale.
[L-02] User blocked by Circle cannot redeem
When instant redeeming either OUSG or rOUSG, at the end the user is funded their USDC.
File: contracts/ousg/ousgInstantManager.sol
455: usdc.transfer(msg.sender, usdcAmountOut);The issue is that Circle (owner of USDC) can add addresses to a blocklist. If the user holding OUSG (or rOUSG) is blocked by Circle, they will never be able to redeem.
Recommendation
Consider adding an address to for redemptions so that they can send the USDC to an address that is not blocked by Circle.
[L-03] usdcReceiver is immutable
Whenever someone mints, their USDC payment is sent to the address usdcReciver.
File: contracts/ousg/ousgInstantManager.sol
319: usdc.transferFrom(msg.sender, usdcReceiver, usdcAmountAfterFee);The issue is that usdcReceiver is immutable:
File: contracts/ousg/ousgInstantManager.sol
90: address public immutable usdcReceiver;Where this address is to be blocked by Circle, minting in ousgInstantManager would stop to work and a new ousgInstantManager would have to be deployed using a new usdcReceiver. This could be confusing for users.
Recommendation
Consider making usdcReceiver mutable and add a way for the protocol to change it.
[L-04] ousgInstantManager::mint/redeem lacks slippage parameter
In ousgInstantManager you can mint either rOUSG or OUSG using USDC and then redeem back to USDC. Both of these use an oracle to track the price of OUSG. This price can vary between when a transaction is sent to when it is executed. This can cause a user to mint or redeem at a different price than they intended.
Recommendation
Consider adding a minOut parameter for ousgInstantManager::mint and redeem calls.
[L-05] Lack of safe transfer wrapper
In ousgInstantManager the admin can make a call to transfer any tokens out of the contract.
ousgInstantManager::retrieveTokens:
File: contracts/ousg/ousgInstantManager.sol
819: function retrieveTokens(
820: address token,
821: address to,
822: uint256 amount
823: ) external onlyRole(DEFAULT_ADMIN_ROLE) {
824: IERC20(token).transfer(to, amount);
825: }Some tokens behave weirdly when transferred and need some extra attention.
Recommendation
Consider using OZ safeTransfer wrapper to transfer tokens for better compatibility with different tokens.
[NC-01] Unnecessary checks
File: contracts/ousg/ousgInstantManager.sol
415: uint256 usdcFees = _getInstantRedemptionFees(usdcAmountToRedeem);
416: usdcAmountOut = usdcAmountToRedeem - usdcFees;
417: require(
418: usdcAmountOut > 0,
419: "OUSGInstantManager::_redeem: redeem amount can't be zero"
420: );usdcAmountOut can never be 0 here, as there is already a check that usdcAmountToRedeem is greater than minimumRedemptionAmount and minimumRedemptionAmount can be at the lowest 10_000.
The redeemFee can also be no larger than 2%. Hence, usdcAmountOut can never be lower than 9_800 (10_000 - ((10_000 * 200) / 10_000)).
The same logic applies to the ousgAmountOut in _mint; however, the math is a bit more complicated since the lowest possible 9_800 usdc is converted to OUSG. However, due to the price having a lower cap, neither this can ever be 0.
Consider removing these two checks.
[NC-02] usdcfees doesn’t follow camelCase
File: contracts/ousg/ousgInstantManager.sol
303: uint256 usdcfees = _getInstantMintFees(usdcAmountIn);Here usdcfees isn’t camel cased. In _redeem the same variable is using camelCase, which is the naming convention used throughout the code.
Consider using camelCase for usdcfees in _mint as well.
[NC-03] Inconsistent address(0) checks
ousgInstantManager::setOracle, setFeeReceiver and setInvestorBasedRateLimiter all let admin change various addresses to external contracts.
However, only setFeeReceiver checks for address(0) before assigning. Consider checking for address(0) in all or none of the calls to have consistent behavior.
[NC-04] Erroneous math in documentation
In the documentation for rOUSG it says rOUSG#L36-L45:
File: contracts/ousg/rOUSG.sol
36: * For example, assume that we have:
37: *
38: * ousgPrice = 100.505
39: * sharesOf(user1) -> 100
40: * sharesOf(user2) -> 400
41: *
42: * Therefore:
43: *
44: * balanceOf(user1) -> 105 tokens which corresponds 105 rOUSG
45: * balanceOf(user2) -> 420 tokens which corresponds 420 rOUSGThis is confusing. As first, one share is one ten-thousandth of a OUSG; it is unclear what a “share” means here. Second, the math is wrong, 100 * 100.505 / 100 = 100.505 and 400 * 100.505 / 100 = 402.02.
Consider updating the documentation.
[NC-05] Misspelled parameter
ousgInstantManager::setInstantRedemptionLimitDuration:
File: contracts/ousg/ousgInstantManager.sol
540: function setInstantRedemptionLimitDuration(
541: uint256 _instantRedemptionLimitDuratioin
542: ) external override onlyRole(CONFIGURER_ROLE) {
543: _setInstantRedemptionLimitDuration(_instantRedemptionLimitDuratioin);
544: }Here _instantRedemptionLimitDuratioin is misspelled, also in the natspec for the call. Consider changing it to _instantRedemptionLimitDuration.
cameronclifton (Ondo) confirmed and commented:
[L-01] - Will not be addressing, the existing functionality is desired.
[L-02] - Will not be addressing.
[L-03] - Addressed,usdcReceiveris now settable.
[L-04] - Will not be addressing.
[L-05] - Will not be addressing.All Non-Criticals - Addressed.
Note: For full discussion, see here.
Disclosures
C4 is an open organization governed by participants in the community.
C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.