c4-bot-5 commented 3 months ago

Lines of code

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

Vulnerability details

Impact

The mintFee and redeemFee variables in the contract OUSGInstantManager.sol are basis points of the fees collected when minting and redeeming OUSG respectively.

There are checks to ensure these fees remain below a certain limit (200) before assignment, these checks and assignment occur in the setMintFee and setRedeemFee functions respectively. The major issue is that the developer seemed to have mixed up the variables that need to be checked before setting the new values.

The current incorrect implementation checks the old mintFee value (L407), the old redeemFee value (L415) rather than the incoming _mintFee and _redeemFee values.

This could be a small yet vital error that could potentially cause a loss of funds, considering it could lead to setting higher fees ṭhan users expect.

Proof of Concept

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

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

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

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

Tools Used

VSCode

Recommended Mitigation Steps

To resolve this issue, correct the checks on the fees so they apply to the newly provided values _mintFee and _redeemFee instead of the old mintFee and redeemFee values. Here is the corrected implementation:

 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;
  }

Assessed type

Invalid Validation

c4-pre-sort commented 3 months ago

0xRobocop marked the issue as duplicate of #181

c4-judge commented 3 months ago

3docSec changed the severity to QA (Quality Assurance)