Missing Return Statement in `_getReserves` Function in `MagicLpAggregator` Contract

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/oracles/aggregators/MagicLpAggregator.sol#L34

Vulnerability details

The MagicLpAggregator contract contains a flaw within the _getReserves function where it fails to return the reserve values fetched from the pair contract. This oversight results in the latestAnswer function always returning zero, which can have severe implications for any systems that depend on this contract for accurate liquidity pool pricing data.

Impact

The missing return statement in the _getReserves function leads to the latestAnswer function always returning zero. This affects any dependent systems or contracts that rely on accurate price data from the MagicLpAggregator contract, as they will receive incorrect information, potentially leading to financial loss or system failure.

Proof of Concept

The _getReserves function in the provided code snippet does not return the fetched reserves:

33:    function _getReserves() internal view virtual returns (uint256, uint256) {
34:        (uint256 baseReserve, uint256 quoteReserve) = pair.getReserves();
35:    }

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/oracles/aggregators/MagicLpAggregator.sol#L33C5-L35C6

Due to the missing return statement, the latestAnswer function uses uninitialized variables for baseReserve and quoteReserve, which default to zero:

function latestAnswer() public view override returns (int256) {
    // ...
    (uint256 baseReserve, uint256 quoteReserve) = _getReserves();
    baseReserve = baseReserve * (10 ** (WAD - baseDecimals)); // baseReserve defaults to 0
    quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals)); // quoteReserve defaults to 0
    // ...
}

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/oracles/aggregators/MagicLpAggregator.sol#L42C1-L44C69

Tools Used

Manual Review

Recommended Mitigation Steps

Update _getReserves()

-    function _getReserves() internal view virtual returns (uint256, uint256) {
+    function _getReserves() internal view virtual returns (uint256 baseReserve, uint256 quoteReserve) {
-        (uint256 baseReserve, uint256 quoteReserve) = pair.getReserves();
+        (baseReserve, quoteReserve) = pair.getReserves();
     }

Assessed type

Other

[0xm3rlin]

confirmed

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[0xCalibur]

Fixed main branch, meanwhile this contest was happenning

https://github.com/Abracadabra-money/abracadabra-money-contracts

[c4-sponsor]

0xCalibur (sponsor) confirmed

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report

[c4-judge]

thereksfour changed the severity to 3 (High Risk)

[trust1995]

Hi,

The impact is that oracle ALWAYS returns price = 0. I argue that this is in line with Medium severity (like 3/4 dups submitted it) because:

• The ProxyOracle contract can always be updated to follow a fixed MagicLPAggregator
• We have no usage of the price feed in the codebase so their verification of the price cannot be measured.
• A price of 0 is considered invalid in Chainlink and most other feeds, the assumed behavior would be to revert. Admin can replace the feed at that time. Loss of funds seems very unlikely, it would have to be that the first time the issue is discovered it is when price of 0 is assumed legitimate and used to handle funds - is that in line with required likelihood for High?

@0xCalibur if you can weight in on any assumptions of integration with the Oracle setup it would be helpful.

[0xCalibur]

There was a mistake, not returning the value in _getReserves. We turned on "error on warnings" during compilation on Foundry config to avoid such issues in the future. This could have been avoided easily.

[thereksfour]

This fact reduces the severity of this issue, will downgrade it to M

A price of 0 is considered invalid in Chainlink and most other feeds, the assumed behavior would be to revert. Admin can replace the feed at that time. Loss of funds seems very unlikely, it would have to be that the first time the issue is discovered it is when price of 0 is assumed legitimate and used to handle funds - is that in line with required likelihood for High?

[c4-judge]

thereksfour changed the severity to 2 (Med Risk)