Abracadabra Mimswap - Trust's results

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L564

Vulnerability details

Description

MagicLP provides a TWAP value which can be accessed via _BASE_PRICE_CUMULATIVE_LAST_

It is updated in the function below:

function _twapUpdate() internal {
    uint32 blockTimestamp = uint32(block.timestamp % 2 ** 32);
    uint32 timeElapsed = blockTimestamp - _BLOCK_TIMESTAMP_LAST_;
    if (timeElapsed > 0 && _BASE_RESERVE_ != 0 && _QUOTE_RESERVE_ != 0) {
        /// @dev It is desired and expected for this value to
        /// overflow once it has hit the max of `type.uint256`.
        unchecked {
            _BASE_PRICE_CUMULATIVE_LAST_ += getMidPrice() * timeElapsed;
        }
    }
    _BLOCK_TIMESTAMP_LAST_ = blockTimestamp;
}

It is updated by any function that changes the reserves, for example:

function _resetTargetAndReserve() internal returns (uint256 baseBalance, uint256 quoteBalance) {
    baseBalance = _BASE_TOKEN_.balanceOf(address(this));
    quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
    if (baseBalance > type(uint112).max || quoteBalance > type(uint112).max) {
        revert ErrOverflow();
    }
    _BASE_RESERVE_ = uint112(baseBalance);
    _QUOTE_RESERVE_ = uint112(quoteBalance);
    _BASE_TARGET_ = uint112(baseBalance);
    _QUOTE_TARGET_ = uint112(quoteBalance);
    _RState_ = uint32(PMMPricing.RState.ONE);
    _twapUpdate();
}

function _setReserve(uint256 baseReserve, uint256 quoteReserve) internal {
    _BASE_RESERVE_ = baseReserve.toUint112();
    _QUOTE_RESERVE_ = quoteReserve.toUint112();
    _twapUpdate();
}

The root cause of the issue is that the TWAP is updated after reserve changes. Since the TWAP multiplies the duration of time since the last update with the new reserves, an attacker has control over the registered price for the entire passed duration.

For reference, the Uniswap and Beanstalk TWAPs are provided below:

• UniswapV2 - priceXCumulativeLast is written and then reserves are changed
• Beanstalk - pumps are updated before the swap operation.

RareSkills details how TWAP operation works here.

Impact

Any application making use of the MagicLP's TWAP to determine token prices will be exploitable.

Proof of Concept

1. At time T0, `BASE_PRICE_CUMULATIVE_LAST = X.
2. Integrating contract records (T0,X)
3. T seconds pass, without swaps, meaning price remained X.
4. A contract queries _BASE_PRICE_CUMULATIVE_LAST_ to execute a large swap(A->B). The swap could be done through MIMswap or any other AMM.
5. Attacker frontruns the query by inflating the A reserves with a large swap. New price is Y >> X.
6. Integrating contract records (T0+T, X + Y * T)
7. When calculating TWAP for last T seconds: (X+Y*T-X)/(T0+T-T0) = Y*T/T = Y. Attacker has succeeded in manipulating the price to Y.
8. The victim performs a losing trade
9. Attacker swaps back (B->A) to get back their tokens minus swap fees, profiting from the price manipulation.

Tools Used

Manual audit

Recommended Mitigation Steps

_twapUpdate() needs to be called before reserves are updated.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L381

Vulnerability details

Description

One of the two key parameters in MagicLP pools is I, which is defined to be the ideal ratio between the two reserves. It is set during MagicLP initialization: _I_ = i;

It is used when performing the initial LP deposit, in buyShares():

if (totalSupply() == 0) {
    // case 1. initial supply
    if (quoteBalance == 0) {
        revert ErrZeroQuoteAmount();
    }
    shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_) ? DecimalMath.divFloor(quoteBalance, _I_) : baseBalance;
    _BASE_TARGET_ = shares.toUint112();
    _QUOTE_TARGET_ = DecimalMath.mulFloor(shares, _I_).toUint112();
    if (_QUOTE_TARGET_ == 0) {
        revert ErrZeroQuoteTarget();
    }
    if (shares <= 2001) {
        revert ErrMintAmountNotEnough();
    }
    _mint(address(0), 1001);
    shares -= 1001;

The QUOTE_TARGET is determined by multiplying the BASE_TARGET with I.

The flaw is in the check below: shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_) ? DecimalMath.divFloor(quoteBalance, _I_) : baseBalance; Essentially there needs to be enough quoteBalance at the I ratio to mint baseBalance shares, if there's not enough then shares are instead determined by dividing the quoteBalance with I. An attacker can abuse the mulFloor() to create a major inconsistency. Suppose quoteBalance = 1, baseBalance = 19999, I = 1e14. Then we have: 1 < 19999 * 1e14 / 1e18 => 1 < 1 => False Therefore shares = 19999 . It sets the targets:

_BASE_TARGET_ = 19999
_QUOTE_TARGET_ = 19999 * 1e14 / 1e18 = 1

The result is the ratio 1:19999, when the intended ratio from I is 1:1000.

Essentially a small rounding error is magnified. The rounding direction should instead be: quoteBalance < DecimalMath.mulCeil(baseBalance, _I_) This would ensure that DecimalMath.divFloor(quoteBalance, _I_) is executed. At this point, when calculating QUOTE_TARGET there will not be a precision error as above (it performs the opposing actions to the divFloor).

An attacker can abuse it by making users perform trades under the assumption I is the effective ratio, however the ratio is actually 2I. The pool's pricing mechanics will be wrong. Note that users will legitimately trust any MagicLP pool created by the Factory as it is supposed to enforce that ratio.

The attack can be performed on another entity's pool right after the init() call, or on a self-created pool. The initial cost for the attack is very small due to the small numbers involved.

Impact

Attacker can initialize a Pool with malicious pricing mechanics that break the assumed invariants of the pool, leading to incorrect pool interactions.

POC

Step by step of the initial buyShares() was provided above. _QUOTE_TARGET_ is used by the pricer:

function getPMMState() public view returns (PMMPricing.PMMState memory state) {
    state.i = _I_;
    state.K = _K_;
    state.B = _BASE_RESERVE_;
    state.Q = _QUOTE_RESERVE_;
    state.B0 = _BASE_TARGET_; // will be calculated in adjustedTarget
    state.Q0 = _QUOTE_TARGET_;
    state.R = PMMPricing.RState(_RState_);
    PMMPricing.adjustedTarget(state);
}
...
function sellBaseToken(PMMState memory state, uint256 payBaseAmount) internal pure returns (uint256 receiveQuoteAmount, RState newR) {
    if (state.R == RState.ONE) {
        // case 1: R=1
        // R falls below one
        receiveQuoteAmount = _ROneSellBaseToken(state, payBaseAmount);
        newR = RState.BELOW_ONE;
    } else if (state.R == RState.ABOVE_ONE) {
        uint256 backToOnePayBase = state.B0 - state.B;
        uint256 backToOneReceiveQuote = state.Q - 
		// @@@@@@@@@@   USED HERE 
		state.Q0;

Note that deposits/withdrawals will continue to apply the bad ratio:

} else if (baseReserve > 0 && quoteReserve > 0) {
    // case 2. normal case
    uint256 baseInputRatio = DecimalMath.divFloor(baseInput, baseReserve);
    uint256 quoteInputRatio = DecimalMath.divFloor(quoteInput, quoteReserve);
    uint256 mintRatio = quoteInputRatio < baseInputRatio ? quoteInputRatio : baseInputRatio;
    shares = DecimalMath.mulFloor(totalSupply(), mintRatio);
    _BASE_TARGET_ = (uint256(_BASE_TARGET_) + DecimalMath.mulFloor(uint256(_BASE_TARGET_), mintRatio)).toUint112();
    _QUOTE_TARGET_ = (uint256(_QUOTE_TARGET_) + DecimalMath.mulFloor(uint256(_QUOTE_TARGET_), mintRatio)).toUint112();

Tools Used

Manual audit

Recommended Mitigation Steps

Use DecimalMaths.mulCeil() to protect against the rounding error.

Assessed type

Math

    function getPMMState() public view returns (PMMPricing.PMMState memory state) {
        state.i = _I_;
        state.K = _K_;
        state.B = _BASE_RESERVE_;
        state.Q = _QUOTE_RESERVE_;
        state.B0 = _BASE_TARGET_; // will be calculated in adjustedTarget
        state.Q0 = _QUOTE_TARGET_;
        state.R = PMMPricing.RState(_RState_);
        PMMPricing.adjustedTarget(state);
    }

    function querySellQuote(
        address trader,
        uint256 payQuoteAmount
    ) public view returns (uint256 receiveBaseAmount, uint256 mtFee, PMMPricing.RState newRState, uint256 newQuoteTarget) {
        PMMPricing.PMMState memory state = getPMMState();
       // call below
        (receiveBaseAmount, newRState) = PMMPricing.sellQuoteToken(state, payQuoteAmount);

        (uint256 lpFeeRate, uint256 mtFeeRate) = _MT_FEE_RATE_MODEL_.getFeeRate(trader, _LP_FEE_RATE_);
        mtFee = DecimalMath.mulFloor(receiveBaseAmount, mtFeeRate);
        receiveBaseAmount = receiveBaseAmount - DecimalMath.mulFloor(receiveBaseAmount, lpFeeRate) - mtFee;
        newQuoteTarget = state.Q0;
    }

   function sellQuoteToken(PMMState memory state, uint256 payQuoteAmount) internal pure returns (uint256 receiveBaseAmount, RState newR) {
        if (state.R == RState.ONE) {
            receiveBaseAmount = _ROneSellQuoteToken(state, payQuoteAmount);
            newR = RState.ABOVE_ONE;
        } else if (state.R == RState.ABOVE_ONE) {
            receiveBaseAmount = _RAboveSellQuoteToken(state, payQuoteAmount);
            newR = RState.ABOVE_ONE;
        } else {
            uint256 backToOnePayQuote = state.Q0 - state.Q;
            uint256 backToOneReceiveBase = state.B - state.B0;
            if (payQuoteAmount < backToOnePayQuote) {
                receiveBaseAmount = _RBelowSellQuoteToken(state, payQuoteAmount);
                newR = RState.BELOW_ONE;
                if (receiveBaseAmount > backToOneReceiveBase) {
                    receiveBaseAmount = backToOneReceiveBase;
                }
            } else if (payQuoteAmount == backToOnePayQuote) {
                receiveBaseAmount = backToOneReceiveBase;
                newR = RState.ONE;
            } else {
                receiveBaseAmount = backToOneReceiveBase + _ROneSellQuoteToken(state, payQuoteAmount - backToOnePayQuote);
                newR = RState.ABOVE_ONE;
            }
        }
    }

    function _RAboveSellQuoteToken(
        PMMState memory state,
        uint256 payQuoteAmount
    )
        internal
        pure
        returns (
            uint256 // receiveBaseToken
        )
    {
       // Using both current price and target price
        return Math._SolveQuadraticFunctionForTrade(state.B0, state.B, payQuoteAmount, DecimalMath.reciprocalFloor(state.i), state.K);
    }

    if (state.R == RState.ONE) {
        // case 1: R=1
        // R falls below one
        receiveQuoteAmount = _ROneSellBaseToken(state, payBaseAmount);
        newR = RState.BELOW_ONE;
    } else if (state.R == RState.ABOVE_ONE) {
        uint256 backToOnePayBase = state.B0 - state.B;
        uint256 backToOneReceiveQuote = state.Q - 
		// @@@@@@@@@@   USED HERE 
		state.Q0;

    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);  <========
    }

    function sellBase(address to) external nonReentrant returns (uint256 receiveQuoteAmount) {
        uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
        uint256 baseInput = baseBalance - uint256(_BASE_RESERVE_);
        uint256 mtFee;
        uint256 newBaseTarget;
        PMMPricing.RState newRState;
        (receiveQuoteAmount, mtFee, newRState, newBaseTarget) = querySellBase(tx.origin, baseInput); <==============
...
    function querySellBase(
        address trader,
        uint256 payBaseAmount
    ) public view returns (uint256 receiveQuoteAmount, uint256 mtFee, PMMPricing.RState newRState, uint256 newBaseTarget) {
        PMMPricing.PMMState memory state = getPMMState(); <========
        (receiveQuoteAmount, newRState) = PMMPricing.sellBaseToken(state, payBaseAmount); <======
...
    function getPMMState() public view returns (PMMPricing.PMMState memory state) {
        state.i = _I_;
        state.K = _K_;
        state.B = _BASE_RESERVE_;
        state.Q = _QUOTE_RESERVE_;
        state.B0 = _BASE_TARGET_; // will be calculated in adjustedTarget
        state.Q0 = _QUOTE_TARGET_;
        state.R = PMMPricing.RState(_RState_);
        PMMPricing.adjustedTarget(state); <======
    }
...
    function adjustedTarget(PMMState memory state) internal pure {
        if (state.R == RState.BELOW_ONE) {
            state.Q0 = Math._SolveQuadraticFunctionForTarget(state.Q, state.B - state.B0, state.i, state.K);
        } else if (state.R == RState.ABOVE_ONE) {
            state.B0 = Math._SolveQuadraticFunctionForTarget(
                state.B,
                state.Q - state.Q0,
                DecimalMath.reciprocalFloor(state.i),
                state.K
            );
        }
    }

    function sellBaseToken(PMMState memory state, uint256 payBaseAmount) internal pure returns (uint256 receiveQuoteAmount, RState newR) {
        if (state.R == RState.ONE) {
            // case 1: R=1
            // R falls below one
            receiveQuoteAmount = _ROneSellBaseToken(state, payBaseAmount); <======
            newR = RState.BELOW_ONE;
...
    function _ROneSellBaseToken(
        PMMState memory state,
        uint256 payBaseAmount
    )
        internal
        pure
        returns (
            uint256 // receiveQuoteToken
        )
    {
        // in theory Q2 <= targetQuoteTokenAmount
        // however when amount is close to 0, precision problems may cause Q2 > targetQuoteTokenAmount
        return Math._SolveQuadraticFunctionForTrade(state.Q0, state.Q0, payBaseAmount, state.i, state.K); <======
    }

Lines of code

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

Vulnerability details

Description

Users are expected to add liquidity through addLiquidity() of the Router. It receives base and quote amounts which are adjusted through _adjustAddLiquidity():

function addLiquidity(
    address lp,
    address to,
    uint256 baseInAmount,
    uint256 quoteInAmount,
    uint256 minimumShares,
    uint256 deadline
) external ensureDeadline(deadline) returns (uint256 baseAdjustedInAmount, uint256 quoteAdjustedInAmount, uint256 shares) {
    (baseAdjustedInAmount, quoteAdjustedInAmount) = _adjustAddLiquidity(lp, baseInAmount, quoteInAmount);
    IMagicLP(lp)._BASE_TOKEN_().safeTransferFrom(msg.sender, lp, baseAdjustedInAmount);
    IMagicLP(lp)._QUOTE_TOKEN_().safeTransferFrom(msg.sender, lp, quoteAdjustedInAmount);
    shares = _addLiquidity(lp, to, minimumShares);
}

The adjustment function has DODOv2 code which determines the effective base:quote ratio and adjusts the higher of the two input values so that no input is wasted. Additionally Abrakadbra inserted the following logic at the start of the function:

(uint256 baseReserve, uint256 quoteReserve) = IMagicLP(lp).getReserves();
uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp)) + baseInAmount;
uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp)) + quoteInAmount;
baseInAmount = baseBalance - baseReserve;
quoteInAmount = quoteBalance - quoteReserve;

The intention is that if the current balance of token X is higher than reserve, the input should be reduced so that only the delta needs to be sent. However the logic is miswritten. The balanceOf() and getReserve() calls have been reversed. The correct logic should be: requested amount = reserves + requested amount - balance Instead it is: requested amount = balance + requested amount - reserves. The higher the current balanceOf(), the higher the final requested amount becomes. This is a critical issue because tokens can be donated to the MagicLP by an attacker, making the victim send more tokens than expected. The POC gives an example of such a sequence.

Essentially this makes addLiquidity(amountX, amountY, minShares) become: (amountX2, amountY2, minShares) where amountX2 > amountX, amountY2 > amountY. By definition this is unauthorized use of the victim's funds.

Impact

Unauthorized spending of victim's tokens, leading to monetary losses.

Proof of Concept

1. MagicLP has (500 USDC, 500 DAI) reserves and balances, 100 shares
2. Victim calls addLiquidity for (1000 USDC, 1000 DAI) , min 200 shares
3. Attacker frontrans and donates (1000 USDC, 500 DAI) to the LP
4. addLiquidity() executes:

• baseInAmount = 1500 + 1000 - 500 = 2000 USDC
• quoteInAmount = 1000 + 1000 - 500 = 1500 DAI
• baseIncreaseRatio = 2000 / 500 = 4
• quoteIncreaseRatio = 1500 / 500 = 3
• quoteAdjustedInAmount = 1500 DAI
• baseAdjustedInAmount = 500 * 3 = 1500 USDC

Therefore LP::buyShares() is called with (1500, 1500) for 200 shares instead of (1000, 1000)

User has now put an extra ($500,$500) of unauthorized funds in the contract. They may or may not be aware of it when the TX is executed. Note that funds put in the LP are obviously in risk of impermanent loss alongside any other economic, systemic and security-related risks. This is a simple example to prove the point but could be tweaked for different tokens (more or less volatile), amounts and ratios.

Tools Used

Manual audit

Recommended Mitigation Steps

Change the usage of getReserves() and balanceOf() in the lines below as described:

(uint256 baseReserve, uint256 quoteReserve) = IMagicLP(lp).getReserves();
uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp)) + baseInAmount;
uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp)) + quoteInAmount;

A great extra invariant check would be to require in addLiquidity() that the results baseAdjustedInAmount, quoteAdjustedInAmount are smaller than the original input amounts respectively.

Note that the issue also occurs in the preview function:

function previewAddLiquidity(
    address lp,
    uint256 baseInAmount,
    uint256 quoteInAmount
) external view returns (uint256 baseAdjustedInAmount, uint256 quoteAdjustedInAmount, uint256 shares) {
    (uint256 baseReserve, uint256 quoteReserve) = IMagicLP(lp).getReserves();
    uint256 baseBalance = IMagicLP(lp)._BASE_TOKEN_().balanceOf(address(lp)) + baseInAmount;
    uint256 quoteBalance = IMagicLP(lp)._QUOTE_TOKEN_().balanceOf(address(lp)) + quoteInAmount;
    baseInAmount = baseBalance - baseReserve;
    quoteInAmount = quoteBalance - quoteReserve;

It should also be fixed here. We view it as the same root cause, but of a lesser end impact.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/blast/BlastOnboardingBoot.sol#L142

Vulnerability details

Description

The BlastOnboardingBoot contract contains the bootstraping funds. After launch it will start a reward staking pool and allow users to claim tokens into it. This is in the launch sequence:

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);

Note that the code approves the staking pool towithdraw from the created LP. The code also provisions setting of the staking contract to another one.

// Just in case we need to change the staking contract after
// the automatic bootstrapping process
function setStaking(LockingMultiRewards _staking) external onlyOwner {
    if (ready) {
        revert ErrCannotChangeOnceReady();
    }
    staking = _staking;
    emit LogStakingChanged(address(_staking));
}

The issue is that this function lacks two actions:

• Resetting the previous staking pool's approval to zero
• More importantly, approving the new staking pool all the bootstrap pool shares

As a result, owner which assumes changing to a new staking pool is perfectly safe, will in fact cause reverts when users try claiming in the new pool. It is still possible to revert back to the old staking pool, but in fact it will never be possible to migrate to a new pool as no other approve() call exists in the contract.

Impact

Loss of assumed functionality of the Onboarding contract in a highly-sensitive area.

Tools Used

Manual audit

Recommended Mitigation Steps

• Approve the new contract
• Revoke approval from the previous contract for good sanitization

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L155

Vulnerability details

Description

The LockingMultiRewards allows anyone to stake MagicLP tokens in return for rewards. LP tokens are sent with either functions below:

function stake(uint256 amount, bool lock_) public whenNotPaused {
    _stakeFor(msg.sender, amount, lock_);
}
/// @notice Locks an existing unlocked balance.
function lock(uint256 amount) public whenNotPaused {
    if (amount == 0) {
        revert ErrZeroAmount();
    }
    _updateRewardsForUser(msg.sender);
    _balances[msg.sender].unlocked -= amount;
    unlockedSupply -= amount;
    _createLock(msg.sender, amount);
}

This will lead to _createLock(), which calculates the nextUnlockTime():

function _createLock(address user, uint256 amount) internal {
    Balances storage bal = _balances[user];
    uint256 _nextUnlockTime = nextUnlockTime();

function nextUnlockTime() public view returns (uint256) {
    return nextEpoch() + lockDuration;
}

Note that nextEpoch() would always return the next 1-week (or reward duration) slot.

function epoch() public view returns (uint256) {
    return (block.timestamp / rewardsDuration) * rewardsDuration;
}
function nextEpoch() public view returns (uint256) {
    return epoch() + rewardsDuration;
}

An issue arises because the user cannot specify the latest desired unlock time. This opens the path for the tokens to be locked for longer than expected, which may have significant impact for users if they need the funds. Consider the following case, where x is week number:

• It is day 7x + 6 (one day from nextEpoch)
• Assumed unlock time is 7x + 7 + lockDuration
• The TX does not execute in next 1 day for any reason (gas price went up, validator does not include TX, etc)
• It is now day 7x+7, another epoch starts. Now nextEpoch is 7x + 14
• Executed unlock time is 7x + 14 + lockDuration

This means user's funds are locked for an additional 7 days more than expected.

Note that delayed execution of a user's TX has always been considered in scope, certainly for Med severity impacts: - https://solodit.xyz/issues/m-13-interactions-with-amms-do-not-use-deadlines-for-operations-code4rena-paraspace-paraspace-contest-git - https://solodit.xyz/issues/m-04-lack-of-deadline-for-uniswap-amm-code4rena-asymmetry-finance-asymmetry-contest-git - https://solodit.xyz/issues/m-03-missing-deadline-param-in-swapexactamountout-allowing-outdated-slippage-and-allow-pending-transaction-to-be-executed-unexpectedly-code4rena-pooltogether-pooltogether-git

Essentially the locking function lacks a deadline parameter similar to swapping functions, and the impact is temporary freeze of funds.

Impact

A user's tokens could be locked for an extended duration beyond their intention and without their control.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider adding a latestUnlockTime deadline parameter for the locking functions.

Assessed type

Timing

Lines of code

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

Vulnerability details

Description

MagicLpAggregator is used to price LP tokens for "closely-tied" underlying tokens. It calculates the price below:

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());
}

The code takes the minimal answer between the underlying oracles and considers all reserves to be worth that amount: return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());

The issue is that any difference in price between the assets represents an easy arbitrage opportunity. Suppose we have tokens (A,B), where real oracle shows:

• A = $0.99
• B = $1

The Pool has 1000000 LP tokens and contains:

• 1000000 A
• 1000000 B

The LP value would calculate as: 0.99 * 2000000 / 1000000 = $1.98 The actual value is: (0.99 * 1000000 + 1 * 1000000) / 1000000 = $1.99

Suppose a platform trades LPs using the aggregator pricing. An attacker could:

• Buy 100,000 LP tokens at $198000
• Withdraw from the pool the underlying shares
• Sell 100,000 A, 100,000 B at $199000
• Profit $1000 from the exchange, when the difference is just $0.01 (this is very common fluctuation even with pegged assets).

The delta comes at the expense of LP holders whose position gets minimized.

Impact

Loss of value due to arbitrage of any platform using MagicLpAggregator pricing.

Tools Used

Manual audit

Recommended Mitigation Steps

Always calculate the value based on the real underlying token value multiplied by amount.

Consider creating two separate oracles for lower-bound and upper-bound results. Then a lending protocol could indeed use the lower-bound for determining collateral value.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L388

Vulnerability details

Description

The LockingMultiRewards contract facilitates distribution of rewards across the remaining epoch duration. The calculation for reward rate is provided below: reward.rewardRate = amount / _remainingRewardTime;

The rate is later multiplied by the duration to get the total rewards for elapsed time, demonstrated below:

function rewardsForDuration(address rewardToken) external view returns (uint256) {
    return _rewardData[rewardToken].rewardRate * rewardsDuration;
}

An issue occurs because there is no sufficient wrapping of the amount before dividing by _remainingRewardTime. The number is divided and later multiplied by elapsed time, causing a loss of precision of the amount modulo remaining time. For the provided period of 1 week by the sponsor, the maximum amount lost can be calculated: 7 * 24 * 3600 - 1 = 604799. Note that the average case loss is half of the worst case assuming even distribution across time. However since rewards are usually not sent at the low end of remaining time (see the minRemainingTime variable, the actual average would be higher).

The effect of this size of loss depends on the decimals and value of the reward token. For USDC, this would be $0.6. For WBTC, it would be $423 (at time of writing). The loss is shared between all stakers relative to their stake amount. The loss occurs for every notification, so it is clear losses will be severe.

Sponsor remarked in the channel that reward tokens would be real, popular tokens. We believe USDC / WBTC on ARB are extremely popular tokens and therefore remain fully in scope as reward tokens.

Severity Rationalization

Impact - high Likelihood - medium -> Severity - high

Impact

Permanent loss of yield for stakers in reward pools due to precision loss.

Proof of Concept

• Reward pool offers WBTC rewards
• Epoch has just started,

Tools Used

Manual audit

Recommended Mitigation Steps

Store the rewardRate scaled by 1e18, so loss of precision will be lower by magnitude of 1e18.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L250

Vulnerability details

Description

The MagicLP contract implements swapping through the sellBase() and sellQuote() functions. It calculates the out amount based on the input amount and the tx.origin:

 (receiveQuoteAmount, mtFee, newRState, newBaseTarget) = querySellBase(tx.origin, baseInput);

The origin is passed as trader to the getFeeRate() function to get the custom lpFeeRate, mtFeeRate for that user:

function querySellBase(
    address trader,
    uint256 payBaseAmount
) public view returns (uint256 receiveQuoteAmount, uint256 mtFee, PMMPricing.RState newRState, uint256 newBaseTarget) {
    PMMPricing.PMMState memory state = getPMMState();
    (receiveQuoteAmount, newRState) = PMMPricing.sellBaseToken(state, payBaseAmount);
	// Passed here @@@@@@@
    (uint256 lpFeeRate, uint256 mtFeeRate) = _MT_FEE_RATE_MODEL_.getFeeRate(trader, _LP_FEE_RATE_);
    mtFee = DecimalMath.mulFloor(receiveQuoteAmount, mtFeeRate);
    receiveQuoteAmount = receiveQuoteAmount - DecimalMath.mulFloor(receiveQuoteAmount, lpFeeRate) - mtFee;
    newBaseTarget = state.B0;
}

The idea to use tx.origin is due to the expected interaction with the contract through the Router periphery contract, in this case the msg.sender is not an accurate representation of the trader.

The issue is that when the trade is executed from a smart contract wallet, for example Gnosis Safe, tx.origin is at the control of the attacker. They may observe the execTransaction() call in the mempool and submit it by themselves, copying the parameters including the signatures. Control of the tx.origin grants the attacker control over the lpFeeRate and mtFeeRate charged in the TX. This means that the victim could assume a certain fee for the swap, while they would be overcharged due to an attacker frontrunning the TX. Note that this attack works both on direct interactions with the MagicLP or TXs through the Router contract.

Impact

When a trader swaps from a smart contract wallet, anyone could make them lose additional value through the trade.

Proof of Concept

1. A smart contract wallet executes a swap through Router's sellBaseTokensForTokens()
2. An attacker views the call in the mempool and executes the call from their own EOA / smart contract.
3. The registered tx.origin for fee determination will be the attacker instead of the real sender.
4. Extra fees will be absorbed by the protocol leading to loss of value for the trader.

Severity Rationalization

• The prerequisite that victim must be a smart contract wallet is not a limiting condition, because it is extremely likely smart contract wallets, or smart contracts integrating with MIMswap, will be used.
• The FeeRateModel implementation example ignores the trader passed in the second argument. However that is not valid grounds for downgrading since the issue is in the MagicLP contract, which cannot make any assumptions on the non-usage of any passed parameters. It is very reasonable to assume the trader parameter would be used, otherwise it wouldn't be passed to the function in a newly-written codebase.

Without severity reduction from above it seems clear the loss of capital translates to high-severity.

Tools Used

Manual audit

Recommended Mitigation Steps

Abolish the use of tx.origin. The Router can pass the msg.sender down to the MagicLP contract to determine the intended trader.

Note that the issue extends also to the flashloan() function, to a lesser impact.

Assessed type

Invalid Validation