Minted Shares would be Inflate Shares by 10000 due to Math Error in the wrap function

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L436

Vulnerability details

Impact

BPS_DENOMINATOR in the rUSDY.sol contract is an important variable used to scale up usdy amount -> shares as noted in it comment description but it is only used to scale up when minting shares but not scaled down before subtracting it from total shares and burning it, this would cause fund math calculation related errors and possible inflation of shares

Proof of Concept

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L436

 function wrap(uint256 _USDYAmount) external whenNotPaused {
    require(_USDYAmount > 0, "rUSDY: can't wrap zero USDY tokens");
--->    _mintShares(msg.sender, _USDYAmount * BPS_DENOMINATOR);
    usdy.transferFrom(msg.sender, address(this), _USDYAmount);
    emit Transfer(address(0), msg.sender, getRUSDYByShares(_USDYAmount));
    emit TransferShares(address(0), msg.sender, _USDYAmount);
  }

The function above shows how the _USDy amouth is scaled up before minting shares however no scale down was done before burning shares as would be shown below https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L449-L453

 function unwrap(uint256 _rUSDYAmount) external whenNotPaused {
    require(_rUSDYAmount > 0, "rUSDY: can't unwrap zero rUSDY tokens");
    uint256 usdyAmount = getSharesByRUSDY(_rUSDYAmount);
    if (usdyAmount < BPS_DENOMINATOR) revert UnwrapTooSmall();
----> _burnShares(msg.sender, usdyAmount);
    usdy.transfer(msg.sender, usdyAmount / BPS_DENOMINATOR);
    emit TokensBurnt(msg.sender, _rUSDYAmount);
  }

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L588

  function _burnShares(
    address _account,
    uint256 _sharesAmount
  ) internal whenNotPaused returns (uint256) {
    require(_account != address(0), "BURN_FROM_THE_ZERO_ADDRESS");

    _beforeTokenTransfer(_account, address(0), _sharesAmount);

    uint256 accountShares = shares[_account];
    require(_sharesAmount <= accountShares, "BURN_AMOUNT_EXCEEDS_BALANCE");

    uint256 preRebaseTokenAmount = getRUSDYByShares(_sharesAmount);

 ---->totalShares -= _sharesAmount;
...

As seen from the two functions above, at no point in time in the unwrap(...) or _burnShares(...) functions was a scale down down with BPS_DENOMINATOR before deducting it from total shares

Tools Used

Manual Review

Recommended Mitigation Steps

Mitigation depends on Sponsor preference, one way could be to ensure a scale down of the shares with BPS_DENOMINATOR before burning the shares in unwrap function call, another way could be to mint the shares without up scaling it in the wrap function call as provided below. multiplication of _USDYAmount with BPS_DENOMINATOR should simply be replaced with just _USDYAmount

 function wrap(uint256 _USDYAmount) external whenNotPaused {
    require(_USDYAmount > 0, "rUSDY: can't wrap zero USDY tokens");
+++ _mintShares(msg.sender, _USDYAmount );
--- _mintShares(msg.sender, _USDYAmount * BPS_DENOMINATOR);
    usdy.transferFrom(msg.sender, address(this), _USDYAmount);
    emit Transfer(address(0), msg.sender, getRUSDYByShares(_USDYAmount));
    emit TransferShares(address(0), msg.sender, _USDYAmount);
  }

or

 function unwrap(uint256 _rUSDYAmount) external whenNotPaused {
    require(_rUSDYAmount > 0, "rUSDY: can't unwrap zero rUSDY tokens");
    uint256 usdyAmount = getSharesByRUSDY(_rUSDYAmount);
    if (usdyAmount < BPS_DENOMINATOR) revert UnwrapTooSmall();
+++  _burnShares(msg.sender, usdyAmount / BPS_DENOMINATOR);
---  _burnShares(msg.sender, usdyAmount);
    usdy.transfer(msg.sender, usdyAmount / BPS_DENOMINATOR);
    emit TokensBurnt(msg.sender, _rUSDYAmount);
  }

Assessed type

Error

[raymondfam]

Scaling down on usdy.transfer(msg.sender, usdyAmount / BPS_DENOMINATOR) would do it just right.

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as duplicate of #46

[c4-judge]

kirk-baird marked the issue as unsatisfactory: Invalid