search

code-423n4 / 2024-03-ondo-finance-findings

5 stars 6 forks source link

Integration issue in ousgInstantManager with BUILD if minUSTokens is set by blackrock. #309

Open c4-bot-10 opened 3 months ago

c4-bot-10 commented 3 months ago

Lines of code

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

Vulnerability details

Impact

Integration issues with BUILD incase blackrock decides to set a minimum amount of BUILD 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 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 BUILD 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);
            }

What this does is it essentially checks that the sender has atleast min amount of tokens after the transfer.

The probem is the ousgInstantManager doesn't require that it always has this much amount of BUILD. When redeeming suppose it has 300k BUILD and the minimum is say 10k BUILD and an investor in ondo wants to redeem 300k BUILD tokens it would still revert even though the contract has it. Which could unexpectedly revert violiating 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 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 BUILD, so 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.

ps.-Forgive me for the bad test code lol ;)

Tools Used

manual, foundry

Recommended Mitigation Steps

Import IDSComplianceConfigurationService or create an interface just for the getMinUSTokens() function and considering replacing the require statement in the _redeemBUILD function with;

 function _redeemBUIDL(uint256 buidlAmountToRedeem) internal { 
    require(
      buidl.balanceOf(address(this)) - IDSComplianceConfigurationService(0x1dc378568cefD4596C5F9f9A14256D8250b56369).getMinUSTokens >= minBUIDLRedeemAmount,  
      "OUSGInstantManager::_redeemBUIDL: Insufficient BUIDL balance" 
    );

so that the contract never tries 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 BUILD 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, implimenting it now should be more compatible with BUILD even in the future if blackrock decides to set it.

Assessed type

Invalid Validation

0xRobocop commented 3 months ago

Integrations with BUIDL contracts are in a grey area concerning scope. Check for example #267 which talks about a potential integration issue with the BUIDL redeem contract.

I will dupe here any integration issue regarding the BUIDL token.

c4-pre-sort commented 3 months ago

0xRobocop marked the issue as insufficient quality report

c4-pre-sort commented 3 months ago

0xRobocop marked the issue as sufficient quality report

c4-pre-sort commented 3 months ago

0xRobocop marked the issue as primary issue

c4-sponsor commented 3 months ago

cameronclifton marked the issue as disagree with severity

cameronclifton commented 3 months ago

Not really a vulnerability (fundamentally this is no different than USDC or BUIDL freezing or seizing their tokens) but I think this is a somewhat interesting finding nonetheless. I will ack this as its in the grey area as "in scope" and it is up to the judges on how they want to treat this. My suggestion is that this should be categorized as low risk if categorized at all.

c4-sponsor commented 3 months ago

cameronclifton (sponsor) acknowledged

asuitouthang commented 3 months ago

Hello sir, I would like to escalate this issue as I think it satisfy for a medium severity.

Example: contract currently holds 1000000 BUILD and 0 USDC. A user wants to redeem 750,000 OUSG, another user tries to frontrun to DOS the first user, he would need 250,001(guarded with redemption fees). However,if blackrock sets minUSTokens to 10k.The attacker only needs minimumRedemptionAmount(50k). After frontrunning with minimumRedemptionAmount, contract now holds 750,000 BUILD, the first user tries to redeem 750,000 BUILD. The redeemption will unexpectedly revert because contract holds less than minUSTokens, violiating core functionality of the protocol(instant redeem). Attacker needs only 50,000 OUSG(which means lesser redemption fees, higher chance for attack) which is significantly lower compared to 250,001 OUSG. So, this issue satisfy the severity categorization by C4 for medium.

3docSec commented 3 months ago

@asuitouthang please refrain from commenting on issues outside of the post-judging-QA period. While you wait for this timeframe, you can review the backstage guidelines that include this and many more guidelines you should follow to keep your backstage access.

3docSec commented 3 months ago

This finding looks like a valid medium to me:

c4-judge commented 3 months ago

3docSec marked the issue as satisfactory

c4-judge commented 3 months ago

3docSec marked the issue as selected for report

cameronclifton commented 3 months ago

We will not mitigate this as of now in the smart contract code as now as we plan to work with the BUIDL team to better understand the conditions in which minUSTokens will be set.

notbozho commented 3 months ago

Hello, @3docSec

This issue suggests an integration gap in the ousgInstantManager.sol contract of the Ondo financial protocol. This flaw is related to the hypothetical scenario where the BUILD token compliance mechanism imposes a minimum holding requirement (minUSTokens) on token holders, which ousgInstantManager fails to account for during redemption operations. However, I think this problem can be classified as low-severity (or even invalid) for several reasons:

1. Hypothetical and Controllable Scenario:

The core of the vulnerability relies on a speculative action that BlackRock might take in the future—activating the minUSTokens mechanism. Currently, this mechanism is not active, and there is no indication that it will be. Moreover, even if activated, such a decision would likely be communicated well in advance, allowing for adjustments to be made.

2. Separation of the concerns:

ousgInstantManager is designed with a specific set of functionalities centered around mining and redeeming OUSG and rOUSG tokens against USDC. It works assuming a standard ERC20 interface and expected behaviors. Extending its responsibility to meet the speculative compliance requirements of another token (BUILD in this case) would violate the separation of concerns principle, leading to increased complexity and the potential for errors in unrelated parts of the system.

3. Modifiability and Upgradeability:

Should the speculated change by BlackRock come to pass, the Ondo Finance Protocol is built with upgradeability in mind. Contracts can be adjusted or extended to comply with new requirements, thus mitigating the impact of such changes. This flexibility reduces the risk associated with the reported bug.

4. Practical solutions exist:

Even in a scenario where the minUSTokens requirement becomes active, there are workarounds. These could include maintaining a buffer of BUILD tokens within ousgInstantManager to satisfy the minimum balance requirement or implementing a check before redemption operations to ensure compliance. These solutions can be implemented without fundamentally changing the operation of the contract or assuming significant risk.

5. Risk Assessment:

The impact of the issue, even if it materializes, is primarily on the availability of the redemption feature under certain conditions, rather than a security vulnerability that could lead to loss of funds or unauthorized access. As such, classifying it as a medium severity bug may overstate the actual risk to the protocol and its users.

6. Known Issues:

Integrations with BUIDL contracts are in a grey area concerning scope. Given the Ondo Finance protocol's acknowledgment of various known issues and their proactive approach to managing potential vulnerabilities, this issue can be seen as having low severity or even being invalid for several key reasons:

6.1. The protocol already employs mechanisms like off-chain agreements and fees to manage risks, indicating a readiness to address potential integration challenges.

6.2. The centralized control structure allows for quick adjustments in response to issues like the minUSTokens setting, reducing the risk of significant impact.

6.3. The team's acknowledgment of external financial dynamics and rebasing issues shows they are well-prepared to adapt to changes, including third-party token compliance mechanisms.

6.4. Given the protocol's handling of KYC and sanctions edge cases, compliance requirements from third-party tokens appear manageable within the existing framework.

6.5. The protocol's openness to addressing third-party integration flaws suggests a willingness and capability to mitigate potential conflicts arising from external token requirements.

These points underscore the Ondo Finance protocol's comprehensive approach to risk management and operational flexibility. Therefore, the concern over BUILD's minUSTokens is likely within the protocol's capacity to manage effectively, justifying its classification as low severity or invalid.

In conclusion, the current impact and severity of this issue is minimal. It represents a controllable and adjustable aspect of the protocol's interaction with external tokens, not an vulnerability. Therefore, classifying this issue as invalid or low severity, with recommendations for monitoring and potential future corrections, is a reasonable.

Also, the fact that the Ondo team won't mitigate this bug and still reward it is unfair and would affect the reward calculation making it unfair compared to other findings. So I strongly believe that low severity is the most adequate in this case.

Have a good one!

asuitouthang commented 3 months ago

Hello Sir, thanks for commenting, however I would like to disagree with you. The reasons you've given to downgrade the issue seems more like external walkarounds for the issue rather than pointing out the issue doesn't exist. I would also like to disagree on the points you mentioned such as known issues, risk assessment, modifiable and upgradeability, etc. And the issue should still be considerd as a medium according to C4, which is also confirmed by the judge.
Also, I dont think that the sponsor not mitigating the issue currently doesn't seem to be valid decision to downgrade the issue, also the sponsor mentioned will not mitigate it **as of now** but plan to work with BUILD team to better understand... which seems to me like it could be considered an approach taken because of the report. And you mentioned, the fact that ondo wont mitigate this and still reward it is unfair, which I would like to disagree because not rewarding this because the sponsor won't mitigate as of now even though it is a valid medium also confirmed by the judge is unfair to me, in addition with the time I spent to find the issue by reviewing the build contract to find integration issues as it is in-scope if there are integration issues with it according to contest details, and also it is not necessary that c4 will only reward issues that are to be mitigated.

Thanks.

3docSec commented 3 months ago

The core of the vulnerability relies on a speculative action that BlackRock might take in the future—activating the minUSTokens mechanism. Currently, this mechanism is not active, and there is no indication that it will be. Moreover, even if activated, such a decision would likely be communicated well in advance, allowing for adjustments to be made.

These are opinions, not facts. The fact is that the code supports activation, and there is no public record of a guarantee that the team does not intend to use it at all - at least, I did not find any - if you find some and share it here, that will change things.

the Ondo Finance Protocol is built with upgradeability in mind. Contracts can be adjusted or extended to comply with new requirements, thus mitigating the impact of such changes

our guidelines are clear that Contract upgradability should never be used as a severity mitigation, i.e. we assume contracts are non-upgradable.

As per point 6 (or point sixes rather), many bizarre ERC20 behaviours are considered, some are in scope, some are out of scope. An integration behaviour should be consider in-scope unless explicitly marked out-of-scope. Having the in-scope list only helps prevent FAQs.

Also, this issue responds well to:

Third party integrations - The BUIDL token and existing BUIDL redeemer contract are not explicitly included and can be assumed to be “correct” for the sake of this audit. However, if one can find flaws in the integration [...], it would be valid

Finally, I will quote our severity categorization:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

This finding is: availability impacted, under stated assumptions and external requirements -> Med

Breeje16 commented 3 months ago

Hi @3docSec,

We are aware that if all BUIDL has been redeemed from our contract, we would not be able to provide instant redemptions for OUSG or rOUSG.

This indicates that maintaining sufficient BUIDL tokens in the contract all the time for redemption is not an invariant at the smart contract level. Rather, it's the admin's role, which lies outside the contest's scope, to uphold this invariant as much as possible.

While I commend the warden's effort for their thorough research here, but based on the above facts, I agree with the sponsor that it is Not really a vulnerability or an integration issue at the smart contract level, as there's no exploitability scenario within the contest scope without a Trusted party's mistake and hence should be a QA.

radeveth commented 3 months ago

Hey @asuitouthang! Really nice finding, but I think it should be classified as a QA finding.

  1. You say the following in your comment here:

    The contest details mentioned: " However, if one can find flaws in the integration or even discover a path for a user (KYC’d or not) to cause a loss of funds for the protocol, it would be valid. " which validates the report as in-scope.

    However, as we can see in the contest README it says the following:

    Ondo Finance will be responsible for ensuring enough BUIDL is in the contract at all times to satisfy investor redemptions.

    That sentence alone invalidates your finding, as well as all your additional comments.

  2. As the sponsor above says, this is not really a vulnerability (fundamentally this is no different than USDC or BUIDL freezing or seizing their tokens). The essence of the security risk is based on a potential move that BlackRock could make in the future implementing the minUSTokens mechanism. At present, this mechanism is inactive, and there are no signs that it will be used. Furthermore, the sponsor states the following above: we plan to work with the BUIDL team to better understand the conditions in which minUSTokens will be set.

@3docSec

asuitouthang commented 3 months ago

Hello, I would argue that the both the first points made by the wardens that the demonstrated exploitability must be within scope is wrong because at the time of the contest the sponsor was not aware of the issue. And the quoted details; We are aware that if all BUIDL has been redeemed from our contract, we would not be able to provide instant redemptions for OUSG or rOUSG and Ondo Finance will be responsible for ensuring enough BUIDL is in the contract at all times to satisfy investor redemptions. These are written in the context of the current codebase, in normal conditions where the sponsor was still not aware of the minUSToken which would be unfair to invalidate issues which arise from the unknown/external incompatible issue which is not mentioned, and the fact that blackrock could set it anytime which is out of the control of ondo and possible availability impact because of it should be considered in scope as mentioned in the contest details.
And the first warden pointed out the recommended sections, while I agree that the given recommendation does not fully mitigate the issue, that was the best I could come up with and it is upto the sponsor to use it or come up with a better mitigation. And the second point made by the second warden which quoted the sponsor's message this is not really a vulnerability (fundamentally this is no different than USDC or BUIDL freezing or seizing their tokens). this was later edited by the sponsor and I would like to disagree that it is quite different from pausing or freezing because it is a minimum amount to be held by its users which should be different from completely freezing the token. And At present, this mechanism is inactive, and there are no signs that it will be used. Furthermore, the sponsor states the following above: we plan to work with the BUIDL team to better understand the conditions in which minUSTokens will be set. I am not sure what the warden tries to say here but the quoted sponsor message "Furthermore, the sponsor states the following above: we plan to work with the BUIDL team to better understand the conditions in which minUSTokens will be set" assures that the sponsor is taking step to mitigate the issue. First warden also mentioned I agree with the sponsor that it is Not really a vulnerability or an integration issue at the smart contract level, as there's no exploitability scenario within the contest scope without a Trusted party's mistake and hence should be a QA I would like to disagree with this(check first escalation message) and still consider it an integration issue even though it might not be a permanent dos which still fits in the medium severity as availability impacted.

Thankyou.

3docSec commented 3 months ago

I would invite wardens to share only facts, and only those that have not been discussed yet. We want to limit noise, and everyone sharing their opinions and countering others' with new opinions, will not help the discussion and certainly will not influence a change to the standing judging.

Breeje16 commented 3 months ago

Hi @3docSec,

I agree with you that there is lot of noice under this finding. I have tried my best to make my points here as accurate as possible, focusing solely on the facts presented in the contest readme and Code4rena's Supreme Court verdicts. I would greatly appreciate it if you could take a moment to review it.

I understand that Handling so many diverse opinions in PJQA is indeed a hard Job. But Thanks for always taking stand based on facts!

radeveth commented 3 months ago

@3docSec, please note what I noted in my comment above:

"However, as we can see in the contest README it says the following:

Ondo Finance will be responsible for ensuring enough BUIDL is in the contract at all times to satisfy investor redemptions.

That sentence alone invalidates your finding, as well as all your additional comments."

Henrychang26 commented 3 months ago

There's no reason to perform this attack since it will result in a net loss for attacker due to fees incurred.

Example: contract currently holds 1000000 BUILD and 0 USDC. A user wants to redeem 750,000 OUSG, another user tries to frontrun to DOS the first user, he would need 250,001(guarded with redemption fees). However,if blackrock sets minUSTokens to 10k.The attacker only needs minimumRedemptionAmount(50k). After frontrunning with minimumRedemptionAmount, contract now holds 750,000 BUILD, the first user tries to redeem 750,000 BUILD. The redeemption will unexpectedly revert because contract holds less than minUSTokens, violiating core functionality of the protocol(instant redeem). Attacker needs only 50,000 OUSG(which means lesser redemption fees, higher chance for attack) which is significantly lower compared to 250,001 OUSG. So, this issue satisfy the severity categorization by C4 for medium.

I agree with @Breeje16. The protocol does not explicitly guarantee instant redemption. Any temporary DoS due insufficient BUIDL token is considered acceptable risk.

My assumption regarding the protocol's rationale for having a redemption limit for each period, is to know how much BUIDL tokens to supply at the beginning of each period. The protocol can simply supply instantRedemptionLimit + minUSTokens to prevent any reverts. Although not ideal, it's a relatively easy workaround. Maybe the sponsor can chime in?

3docSec commented 3 months ago

Hi, the README context:

Ondo Finance will be responsible for ensuring enough BUIDL is in the contract at all times to satisfy investor redemptions.

was taken into account for judging and did not exclude this finding, because it's an informative sentence for giving context about how the sponsor plans to operate the protocol. Which is different from saying "Out of scope: anything related to low BUIDL balance".

It's a technicality, it's likely a situation the sponsor intended to cover with their "known issues" phrasing, but the finding highlights one that is not covered by the "known issues" phrasing and is fair to receive a reward consequently.