code-423n4 / 2023-10-badger-findings

1 stars 1 forks source link

Rounding error may result in not properly updating of BaseRateFromRedemption #255

Closed c4-submissions closed 11 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/CdpManager.sol#L621

Vulnerability details

Impact

If _totalEBTCSupply > _ETHDrawn or _price it may result in truncation error , since there is no access control on the function someone having access to it can make the input values, they can put any arguments value

Proof of Concept

for example i>:

_totalEBTCSupply = 10 _ETHDrawn = 5 _price = 2 then redeemedEBTCFraction becomes 1

for example ii>:

_totalEBTCSupply = 11 _ETHDrawn = 4 _price = 2 then redeemedEBTCFraction becomes 0

function _updateBaseRateFromRedemption(
        uint256 _ETHDrawn,
        uint256 _price,
        uint256 _totalEBTCSupply
    ) internal returns (uint256) {
        uint256 decayedBaseRate = _calcDecayedBaseRate();

        /* Convert the drawn ETH back to EBTC at face value rate (1 EBTC:1 USD), in order to get
         * the fraction of total supply that was redeemed at face value. */
        uint256 redeemedEBTCFraction = (collateral.getPooledEthByShares(_ETHDrawn) * _price) /
            _totalEBTCSupply; //@audit

        uint256 newBaseRate = decayedBaseRate + (redeemedEBTCFraction / beta);
        newBaseRate = EbtcMath._min(newBaseRate, DECIMAL_PRECISION); // cap baseRate at a maximum of 100%
        require(newBaseRate > 0, "CdpManager: new baseRate is zero!"); // Base rate is always non-zero after redemption

        // Update the baseRate state variable
        baseRate = newBaseRate;
        emit BaseRateUpdated(newBaseRate);

        _updateLastRedemptionTimestamp();

        return newBaseRate;
    }

Due to this newBaseRate will change, since beta = 2 & this might also result in truncation error. So baseRate will be updated wrongly.

Tools Used

VS Code

Recommended Mitigation Steps

Add a modifier check only for specific admin to access the function for entering the correct values or else add a check for _totalEBTCSupply for not resulting in truncation error.

Assessed type

Math

c4-pre-sort commented 11 months ago

bytes032 marked the issue as insufficient quality report

c4-sponsor commented 11 months ago

GalloDaSballo (sponsor) disputed

GalloDaSballo commented 11 months ago

Disagree that it has any impact

jhsagd76 commented 11 months ago

sounds like it might work when cold start. But definitely not a high. pls provide a poc which shows reward lost > 1 eth

c4-judge commented 11 months ago

jhsagd76 marked the issue as unsatisfactory: Invalid