code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-02-malt/blob/main/contracts/DataFeed/MaltDataLab.sol#L448-L460 https://github.com/code-423n4/2023-02-malt/blob/main/contracts/DataFeed/MaltDataLab.sol#L602-L612

Vulnerability details

Impact

MaltDataLab.getActualPriceTarget() reverts when breakpointBps = 10000.

Proof of Concept

In setBreakpointBps(), it's possible breakpointBps = 10000.

File: 2023-02-malt\contracts\DataFeed\MaltDataLab.sol
602:   function setBreakpointBps(uint256 _breakpointBps)
603:     external
604:     onlyRoleMalt(ADMIN_ROLE, "Must have admin role")
605:   {
606:     require(_breakpointBps != 0, "Cannot have 0 breakpoint BPS");
607:     require(
608:       _breakpointBps <= 10000,
609:       "Cannot have a breakpoint BPS greater than 10,000"
610:     );
611:     breakpointBps = _breakpointBps;
612:   }

And in getActualPriceTarget, it calculates m like the below.

File: 2023-02-malt\contracts\DataFeed\MaltDataLab.sol
449:     int128 breakpointInt = ABDKMath64x64.div( //@audit-info purchaseParityInt * breakpointBps / 10000
450:       ABDKMath64x64.mul(
451:         purchaseParityInt,
452:         ABDKMath64x64.fromUInt(breakpointBps)
453:       ),
454:       ABDKMath64x64.fromUInt(10000)
455:     );
456:     int128 oneInt = ABDKMath64x64.fromUInt(1);
457: 
458:     int128 m = (icTotalInt.sub(oneInt)).div( //@audit-info (icTotalInt - 1) / (purchaseParityInt - breakpointInt)
459:       purchaseParityInt.sub(breakpointInt) //@audit revert when breakpointBps = 10000
460:     );

As we can see from the comments, purchaseParityInt = breakpointInt when breakpointBps = 10000 and the ABDKMath64x64 library reverts on zero division here.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend setting breakpointBps < 10000 always in setBreakpointBps() function.

c4-sponsor commented 1 year ago

0xScotch marked the issue as sponsor confirmed

Picodes commented 1 year ago

Downgrading to low as this would be an additional safety check on an admin setter.

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)