c4-bot-3 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/Vault.sol#L153-L158

Vulnerability details

Description

The Vault::rebalance() function rounds-down the sharesToMint against the protocol's favour. It ought to be rounded-up to avoid loss of funds for the protocol.

                    uint256 feeInEthScaled = uint256(balanceChange) *
                        settings().getPerformanceFee();
                    uint256 sharesToMint = (feeInEthScaled * totalSupply()) /
                        _totalAssets(maxPriceAge) /
                        PERCENTAGE_PRECISION;
                    _mint(settings().getFeeReceiver(), sharesToMint);

Impact

Loss of funds for the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

Round up in favour of the protocol. A library like solmate can be used which has mulDivUp:

-                   uint256 sharesToMint = (feeInEthScaled * totalSupply()) /
-                       _totalAssets(maxPriceAge) /
-                       PERCENTAGE_PRECISION;
+                   uint256 sharesToMint = feeInEthScaled.mulDivUp(totalSupply(), _totalAssets(maxPriceAge) * PERCENTAGE_PRECISION);

Assessed type

Math

c4-judge commented 5 months ago

0xleastwood marked the issue as primary issue

0xleastwood commented 5 months ago

This is not medium severity, there is minimal rounding here that does not cause significant leakage of funds. Downgrading to QA because rounding should always be done in favour of the protocol.

c4-judge commented 5 months ago

0xleastwood changed the severity to QA (Quality Assurance)

t0x1cC0de commented 5 months ago

Hi, Please refer my comment under Issue 4 here.

Thanks

ickas commented 5 months ago

Fixed → https://github.com/baker-fi/bakerfi-contracts/pull/52