Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential Reduction in Instant Minting and Redemption Limits due to Fee Incorporation #312

Open
c4-bot-7 opened this issue Apr 3, 2024 · 3 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-47 grade-b Q-08 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_20_group AI based duplicate group recommendation

Comments

@c4-bot-7
Copy link
Contributor

c4-bot-7 commented Apr 3, 2024

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L278
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L388
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/InstantMintTimeBasedRateLimiter.sol#L93
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/InstantMintTimeBasedRateLimiter.sol#L120

Vulnerability details

Impact

_checkAndUpdateInstantMintLimit() and _checkAndUpdateInstantRedemptionLimit() include fees, which could lead to a potential reduction in allowable amounts by up to 1.99% in each window.

Proof of Concept

Users have the ability to instant mint() and redeem() OUSG in exchange for USDC. _checkAndUpdateInstantMintLimit() and _checkAndUpdateInstantRedemptionLimit() are responsible for limiting the amount permitted in the current window.

  function _checkAndUpdateInstantMintLimit(uint256 amount) internal {
    require(amount > 0, "RateLimit: mint amount can't be zero");

    if (
      block.timestamp >= lastResetInstantMintTime + resetInstantMintDuration
    ) {
      // time has passed, reset
      currentInstantMintAmount = 0;
      lastResetInstantMintTime = block.timestamp;
    }
    require(
      amount <= instantMintLimit - currentInstantMintAmount,
      "RateLimit: Mint exceeds rate limit"
    );

    currentInstantMintAmount += amount;
  }
  function _checkAndUpdateInstantRedemptionLimit(uint256 amount) internal {
    require(amount > 0, "RateLimit: redemption amount can't be zero");

    if (
      block.timestamp >=
      lastResetInstantRedemptionTime + resetInstantRedemptionDuration
    ) {
      // time has passed, reset
      currentInstantRedemptionAmount = 0;
      lastResetInstantRedemptionTime = block.timestamp;
    }
    require(
      amount <= instantRedemptionLimit - currentInstantRedemptionAmount,
      "RateLimit: Redemption exceeds rate limit"
    );
    currentInstantRedemptionAmount += amount;
  }

In both _mint() and _redeem(), the protocol has the option to impose fees. These fees are deducted from usdcAmountIn and usdcAmountToRedeem respectively in each function. However, these fees are factored into _checkAndUpdateInstantMintLimit() and _checkAndUpdateInstantRedemptionLimit(), potentially resulting in a reduction of the allowable amount in each window by up to 1.99%.

Tools Used

Manual Review

Recommended Mitigation Steps

  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;
+   _checkAndUpdateInstantMintLimit(usdcAmountAfterFee);

    uint256 ousgPrice = getOUSGPrice(); 
    ousgAmountOut = _getMintAmount(usdcAmountAfterFee, ousgPrice);

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

    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);
  }
  function _redeem(
    uint256 ousgAmountIn
  ) internal returns (uint256 usdcAmountOut) {
    require(
      IERC20Metadata(address(usdc)).decimals() == 6,
      "OUSGInstantManager::_redeem: USDC decimals must be 6"
    );
    require(
      IERC20Metadata(address(buidl)).decimals() == 6,
      "OUSGInstantManager::_redeem: BUIDL decimals must be 6"
    );
    uint256 ousgPrice = getOUSGPrice();
    uint256 usdcAmountToRedeem = _getRedemptionAmount(ousgAmountIn, ousgPrice);

    require(
      usdcAmountToRedeem >= minimumRedemptionAmount,
      "OUSGInstantManager::_redeem: Redemption amount too small"
    );
-   _checkAndUpdateInstantRedemptionLimit(usdcAmountToRedeem);

    if (address(investorBasedRateLimiter) != address(0)) {
      investorBasedRateLimiter.checkAndUpdateRedeemLimit(
        msg.sender,
        usdcAmountToRedeem
      );
    }

    uint256 usdcFees = _getInstantRedemptionFees(usdcAmountToRedeem);
    usdcAmountOut = usdcAmountToRedeem - usdcFees;
+   _checkAndUpdateInstantRedemptionLimit(usdcAmountOut);
    require(
      usdcAmountOut > 0,
      "OUSGInstantManager::_redeem: redeem amount can't be zero"
    );

    ousg.burn(ousgAmountIn);

    uint256 usdcBalance = usdc.balanceOf(address(this));

    if (usdcAmountToRedeem >= minBUIDLRedeemAmount) {
      _redeemBUIDL(usdcAmountToRedeem);
    } else if (usdcAmountToRedeem > usdcBalance) {
      _redeemBUIDL(minBUIDLRedeemAmount);
      emit MinimumBUIDLRedemption(
        msg.sender,
        minBUIDLRedeemAmount,
        usdcBalance + minBUIDLRedeemAmount - usdcAmountToRedeem
      );
    } else {
      emit BUIDLRedemptionSkipped(
        msg.sender,
        usdcAmountToRedeem,
        usdcBalance - usdcAmountToRedeem
      );
    }

    if (usdcFees > 0) {
      usdc.transfer(feeReceiver, usdcFees);
    }
    emit RedeemFeesDeducted(msg.sender, feeReceiver, usdcFees, usdcAmountOut);

    usdc.transfer(msg.sender, usdcAmountOut);
  }

Assessed type

Context

@c4-bot-7 c4-bot-7 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Apr 3, 2024
c4-bot-2 added a commit that referenced this issue Apr 3, 2024
@c4-bot-11 c4-bot-11 added the 🤖_20_group AI based duplicate group recommendation label Apr 3, 2024
@c4-pre-sort
Copy link

0xRobocop marked the issue as duplicate of #47

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Apr 9, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Apr 9, 2024

3docSec changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

c4-judge commented Apr 9, 2024

3docSec marked the issue as grade-b

@C4-Staff C4-Staff reopened this Apr 15, 2024
@C4-Staff C4-Staff added the Q-08 label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-47 grade-b Q-08 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_20_group AI based duplicate group recommendation
Projects
None yet
Development

No branches or pull requests

5 participants