search

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

5 stars 6 forks source link

Early users can mint & redeem without paying fee resulting in loss to the protocol #163

Open c4-bot-5 opened 5 months ago

c4-bot-5 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L98-L102 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L554 https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/ousgInstantManager.sol#L567

Vulnerability details

Users can mint & redeem OUSG and rOUSG against USDC in the OUSGInstantManager contract. During this process, fee is collected from the user which can be same or different for either the minting or redeeming upto a certain limit. The fee can also be 0 if the protocol chooses so.

However both the minting & the redeeming fee is set to 0 at the time of deployment allowing early users to have an upper hand against the rest of the users.

Given that there is no documentation regarding this behaviour in the publicly known issues or intended by the developers & that the loss can be huge, the given issue comes under Medium severity.

Proof of Concept

The fee is set to 0 initially.

File: OUSGInstantManager.sol

  // Fee collected when minting OUSG (in basis points)
  uint256 public mintFee = 0;

  // Fee collected when redeeming OUSG (in basis points)
  uint256 public redeemFee = 0;

The fee can be set by a trusted address with an upper limit as can be seen in the require statements.

File: OUSGInstantManager.sol

  function setMintFee(
    uint256 _mintFee
  ) external override onlyRole(CONFIGURER_ROLE) {
    require(mintFee < 200, "OUSGInstantManager::setMintFee: Fee too high");
    emit MintFeeSet(mintFee, _mintFee);
    mintFee = _mintFee;
  }

  function setRedeemFee(
    uint256 _redeemFee
  ) external override onlyRole(CONFIGURER_ROLE) {
    require(redeemFee < 200, "OUSGInstantManager::setRedeemFee: Fee too high");
    emit RedeemFeeSet(redeemFee, _redeemFee);
    redeemFee = _redeemFee;
  }

However inside the constructor both the fees are not set & will be 0 until the set functions are called by the configurer. This would allow early users to mint/redeem tokens without paying anything to the protocol which would be a loss to the protocol.

Loss to the protocol is proportional to the amount of tokens being minted/redeemed.

In the event if an attacker carried out a flashloan with a huge amount of usdc to mint & redeem tokens, this would result in a huge loss to the protocol because the fee is set to 0 at this point.

Impact

Loss of funds to the protocol because no fee is assigned for minting & redeeming.

Tools Used

Manual Review

Recommended Mitigation Steps

Set the minting & redeeming fee inside the constructor.

Assessed type

Error

c4-pre-sort commented 5 months ago

0xRobocop marked the issue as duplicate of #276

c4-judge commented 5 months ago

3docSec changed the severity to QA (Quality Assurance)

c4-judge commented 5 months ago

3docSec marked the issue as grade-b

Shubh0412 commented 5 months ago

Hello @3docSec

The mentioned issue falls falls under Medium severity because of the following reasons :-

  1. The protocol is unable to collect fee while both minting & redeeming when the contract is deployed. There is no way to charge those early depositors later because the fee is set for everyone & not per individual.
  2. Just like the first depositor inflation attack vector which causes loss of funds to the depositor, this above finding lies in the same field where early users can front-run & mint/redeem before the fee is set by the admin, causing loss of funds to the protocol.
  3. Given that the minimum amount to mint is 100k, the portion of fee not collected is definitely greater than dust amount.
3docSec commented 5 months ago

Hi, there's a profound difference between the "first depositor inflation attack" that affects AMMs deployed in a permissionless manner, and this issue, which rather relates to a contract in a centralized protocol.

In this context of a centralized protocol, I would say that if admins intend to operate the protocol with a non-zero fee, it would be their mistake to not set these values transactionally with the deployment, and KYCing addresses before the contract is fully configured. The scenario this finding presents requires therefore an admin mistake, which alone is already sufficient for a QA judgment.

Risk is further mitigated by the fact that users are KYC'ed and admins can also remove them from the protocol and seize assets if they misbehave. This means that, with the attack you mentioned, one would risk 100% of their funds for front-running a max 2% fee

Shubh0412 commented 5 months ago

@3docSec Thank you for explaining the details behind your judgement.

Given the fact that the fee will definitely be 0 at the time of deployment & is later configurable by the admin, leaves the possibility of a bug. But given your reasoning about the time when users are being KYC'ed, thats upto the protocol for decide.

Will leave the final decision in your hands 👍

3docSec commented 5 months ago

Thanks, QA is the final decision 👍