code-423n4 / 2024-03-dittoeth-findings

0 stars 0 forks source link

Redeemers pay low redemption fees due to unsafe downcasting. #235

Closed c4-bot-9 closed 5 months ago

c4-bot-9 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L401-L402

Vulnerability details

Impact

    uint256 redemptionRate = LibOrders.min((Asset.baseRate + 0.005 ether), 1 
    ether);
    return uint88(redemptionRate.mul(colRedeemed));

The code above shows how redemption fee is calculated. redemptionrate ( uint 256) * colredeemed (uint88 ). Now Dittoeth utilizes prbmath for calculations. In the above case the mul function for uint88 is implemented as follows.https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/PRBMathHelper.sol#L114-L116

  function mul(uint88 x, uint256 y) internal pure returns (uint256 result) {
    result = mulDiv18(x, y);
}

As you can see it returns a uint256 result (redemption fee) which is then unsafely casted to uint88. This leads to an overflow if the value if this result is quite large . meaning the value of the redemption fee returned would be much less than the correct one. The purpose of redemption fees in dittoeth is to offer stability by throttling the rate of redemptions and due to the issue described , this purpose is slightly defeated.

Proof of Concept

see above

Tools Used

manual review

Recommended Mitigation Steps

Recommend modifying the code to this ; return uint88(redemptionRate.mulU88(colRedeemed)); this is because mulU88 function checks for overflow;

function mulU88(uint88 x, uint256 y) internal pure returns (uint88 result) {
    uint256 _result = mulDiv18(x, y);
    if (_result > type(uint88).max) revert Errors.InvalidAmount(); // assume amount?
    result = uint88(_result);
}

Assessed type

Under/Overflow

c4-pre-sort commented 5 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #22

raymondfam commented 5 months ago

Similar to #22.

c4-judge commented 4 months ago

hansfriese changed the severity to QA (Quality Assurance)

c4-judge commented 4 months ago

hansfriese marked the issue as grade-c