Abracadabra Mimswap - DarkTower's results

Lines of code

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

Vulnerability details

Impact

MagicLpAggregator does not update the roundId, startAt, updatedAt and answeredInRound to correct values.

MagicLpAggregator.sol#L48-L50

    function latestRoundData() external view returns (uint80, int256, uint256, uint256, uint80) {
        return (0, latestAnswer(), 0, 0, 0);
    }

A common code is to check updatedAt for staleness issue (although it isn't required to do so anymore.

(, int256 price, , uint256 updatedAt, ) = priceFeed.latestRoundData();

if (updatedAt < block.timestamp - 60 * 60 /* 1 hour */) {
   revert("stale price feed");
}

Therefore any integrator that uses the above code will not be able to integrate MagicLpAggregator oracles as it will always revert due to the incorrect updatedAt being provided.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the values roundId, startAt, updatedAt and answeredInRound from whichever oracle, baseOracle or quoteOracle was used.

Assessed type

Other

function testProtocolAIntegrationOfMagicLPAggReverts() public {

        // return values of `MagicLpAggregator` latestRoundData()
        (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound) = aggregator.latestRoundData();

        // protocol A tries to make sure the prices returned is fresh by checking the time

        // keep in mind the time aka updatedAt is hardcoded to 0. So 0 will always be less than `block.timestamp - 60 * 60` hence a revert

        if (updatedAt < block.timestamp - 60 * 60 /* 1 hour */) {
         revert("stale price feed");
        }
    }

Ran 1 test for test/MagicLpAggregator.t.sol:MagicLpAggregatorTest
[FAIL. Reason: revert: stale price feed] testProtocolAIntegrationOfMagicLPAggReverts() (gas: 57187)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.56s (2.88ms CPU time)

Lines of code

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

Vulnerability details

Impact

MagicLPAggregator will return wrong price for tokens less than 18 decimals.

The MagicLPAggregator intends to return price of a LP in terms of USD in 18 decimals.

MagicLpAggregator.sol#L29-L31

 function decimals() external pure override returns (uint8) {
        return 18;
}

But let's take a look at the following function. MagicLpAggregator.sol#L37-L46

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

Let us break down the above function, it first:

• obtains the price returned by the baseOracle
• obtains the price returned by the quoteOracle
• finds the minimum price returned by the two and store in minAnswer
• obtains the total baseReserve + quoteReserve of the MagicLP
• takes minAnswer * (baseReserve + quoteReserve) / pair.totalSupply()
• note: pair.totalSupply() is the total number of MagicLP shares in the pool.

But if you look at the above closely, you will notice that although the function seems correct it outputs the price in the wrong decimals (not 18):

• We know that minAnswer will always be the normalized oracle price to 18 decimals

• (baseReserve + quoteReserve) will always be the normalized token prices to 18 decimals

• However, pair.totalSupply() is not normalized, and depends on the decimals of the base token!

MagicLP.sol#L163-L165

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

Therefore if IERC20Metadata(_BASE_TOKEN_).decimals() is not 18 decimals, let us say 6 decimals (ie. USDT token)

Than the decimals for the final output isn't correct as 18 + 18 - 6 = 30 decimals which is 10^12 times the intended price we wanted to return.

Tools Used

Manual Review

Recommended Mitigation Steps

Normalize pair.totalSupply() to 18 decimals so that the final output is 18 + 18 - 18 = 18 decimals.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

The Abracadabra team configures gas yield for all contracts in the Blast L2 except for the LockingMultiRewards contract. Therefore, gas yields accrued on the LockingMultiRewards staking contract since deployment will be lost as it was never configured using BlastYields.configureDefaultClaimables. If the contract holds native yield reward tokens in ERC20 such as WETHRebasing or USDB then these rewards will also be lost.

Proof of Concept

During deployment of the staking contract on Blast, the staking contract's gas yield mode should be configured to be able to claim accrued gas fees spent on the contract. We know that the bootstrap() function spawns the LockingMultiRewards staking contract below.

function bootstrap(uint256 minAmountOut) external onlyOwner onlyState(State.Closed) returns (address, address, uint256) {
        ...
 @>     staking = new LockingMultiRewards(pool, 30_000, 7 days, 13 weeks, address(this)); // spawns the staking contract
        ...
    }

However, in the LockingMultiRewards contract, the BlastYields.configureDefaultClaimables function does not exist in the LockingMultiRewards constructor to configure the staking contract's gas yield mode. Therefore, gas yield cannot be claimed LockingMultiRewards.

constructor(
        address _stakingToken,
        uint256 _lockingBoostMultiplerInBips,
        uint256 _rewardsDuration,
        uint256 _lockDuration,
        address _owner
    ) OperatableV2(_owner) {
        if (_lockingBoostMultiplerInBips <= BIPS) {
            revert ErrInvalidBoostMultiplier();
        }

        if (_lockDuration < MIN_LOCK_DURATION) {
            revert ErrInvalidLockDuration();
        }

        if (_rewardsDuration < MIN_REWARDS_DURATION) {
            revert ErrInvalidRewardDuration();
        }

        if (_lockDuration % _rewardsDuration != 0) {
            revert ErrInvalidDurationRatio();
        }

        stakingToken = _stakingToken;
        lockingBoostMultiplerInBips = _lockingBoostMultiplerInBips;
        rewardsDuration = _rewardsDuration;
        lockDuration = _lockDuration;

        // kocks are combined into the same `rewardsDuration` epoch. So, if
        // a user stake with locking every `rewardsDuration` this should reach the
        // maximum number of possible simultaneous because the first lock gets expired,
        // freeing up a slot.
        maxLocks = _lockDuration / _rewardsDuration;
    }

We believe this to be a mistake by the Abracadabra team as all other contracts that use substantial gas except for this contract had their gas yield mode configured/ The gas spent for this contract will be very substantial considering that users will spend substantial amounts of gas claiming from the staking contract. Therefore the Abracadabra is missing out the gas fees in this contract by not configuring the gas yield mode and possibly native yield in such as WETHRebasing or USDB if such tokens are used

Tools Used

Manual review + foundry

Recommended Mitigation Steps

Configure the Blast gas yield mode for the LockingMultiRewards staking contract in the constructor and expose a function for admin to collect the yields. Additionally, also consider whether Blast points need to be configured here.

Assessed type

Other

staking = new BlastLockingMultiRewards(registry, feeTo, pool, 30_000, 7 days, 13 weeks, address(this));

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/blast/BlastOnboarding.sol#L101-L121

Vulnerability details

Impact

The bootstrap process relies on users locking up all their tokens as it will only use tokens that are marked locked in totals[MIM].locked and totals[USDB].locked.

BlastOnboardingBoot.sol#L96-L127

    function bootstrap(uint256 minAmountOut) external onlyOwner onlyState(State.Closed) returns (address, address, uint256) {
        if (pool != address(0)) {
            revert ErrAlreadyBootstrapped();
        }

=>      uint256 baseAmount = totals[MIM].locked;
=>      uint256 quoteAmount = totals[USDB].locked;
        MIM.safeApprove(address(router), type(uint256).max);
        USDB.safeApprove(address(router), type(uint256).max);

        (pool, totalPoolShares) = router.createPool(MIM, USDB, FEE_RATE, I, K, address(this), baseAmount, quoteAmount);

        if (totalPoolShares < minAmountOut) {
            revert ErrInsufficientAmountOut();
        }

        // Create staking contract
        // 3x boosting for locker, 7 days reward duration, 13 weeks lp locking
        // make this contract temporary the owner the set it as an operator
        // for permissionned `stakeFor` during the claiming process and then
        // transfer the ownership to the onboarding owner.
        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);
    }

However, it is possible that these values can be zero because there is a cap on the amount of tokens that can be stored in the contract.

BlastOnboarding.sol#L101-L121

    function deposit(address token, uint256 amount, bool lock_) external whenNotPaused onlyState(State.Opened) onlySupportedTokens(token) {
        token.safeTransferFrom(msg.sender, address(this), amount);

        if (lock_) {
            totals[token].locked += amount;
            balances[msg.sender][token].locked += amount;
        } else {
            totals[token].unlocked += amount;
            balances[msg.sender][token].unlocked += amount;
        }

        totals[token].total += amount;

        if (caps[token] > 0 && totals[token].total > caps[token]) {
            revert ErrCapReached();
        }

        balances[msg.sender][token].total += amount;

        emit LogDeposit(msg.sender, token, amount, lock_);
    }

In the code above, notice that there is a cap on the amount of tokens that can be sent to the contract caps[token]. The problem here is that it is checked against the total token amount totals[token].total rather than the locked amount totals[token].locked.

Therefore a griefer can deposit tokens up to the caps[token] with lock_ = false. The result, is that no one else can deposit tokens into the contract.

During bootstrap, since these tokens are still considered unlocked, then totals[MIM].locked = 0 and totals[USDB].locked = 0, therefore there won't be any locked tokens available for the bootstrapping process.

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of checking caps[token] against totals[token].total, it should be checked against totals[token].locked.

Assessed type

Other

[L-01]: Removing supported tokens prevents users from withdrawing tokens from BlastOnboarding.

Due to the onlySupportedTokens(token) modifier, if the owner removes support for tokens, users can no longer withdraw their tokens and their funds will get stuck in the BlastOnboarding.

BlastOnboarding.sol#L132-L141

    function withdraw(address token, uint256 amount) external whenNotPaused onlySupportedTokens(token) {
        balances[msg.sender][token].unlocked -= amount;
        balances[msg.sender][token].total -= amount;
        totals[token].unlocked -= amount;
        totals[token].total -= amount;

        token.safeTransfer(msg.sender, amount);

        emit LogWithdraw(msg.sender, token, amount);
    }

Rescues will also not be able function for unsupported tokens.

BlastOnboarding.sol#L205-L212

    function rescue(address token, address to, uint256 amount) external onlyOwner {
        if (supportedTokens[token]) {
            revert ErrNotAllowed();
        }

        token.safeTransfer(to, amount);
        emit LogTokenRescue(token, to, amount);
    }

Therefore, these tokens will get stuck in the contract until it supports these tokens again which may not be ideal.

Consider removing onlySupportedTokens(token) modifier for withdrawals and rescues

[L-02]: FeeRateModel.sol, FeeRateModelImpl.sol, BlastTokenRegistry.sol do not configure gas yield.

The constructors for the following contracts do not configure gas yield on Blast:

1. https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/blast/BlastTokenRegistry.sol

2. https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/auxiliary/FeeRateModel.sol

3. https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/auxiliary/FeeRateModelImpl.sol

Gas yield for these files won't be substantial so this is marked as low.

[L-03]: BlastMagicLP implementation do not configure gas yield.

The implementation BlastMagicLP does not configure the gas yield mode in the constructor.

BlastMagicLP.sol#L23-L33

    constructor(BlastTokenRegistry registry_, address feeTo_, address owner_) MagicLP(owner_) {
        if (feeTo_ == address(0)) {
            revert ErrZeroAddress();
        }
        if (address(registry_) == address(0)) {
            revert ErrZeroAddress();
        }

        registry = registry_;
        feeTo = feeTo_;
    }

Since there will still be operations carried out on the implementation contract for admin configuration purposes, it might be best to configure the yield mode by calling BlastYields.configureDefaultClaimables(address(this)) in the constructor so that any gas spent on implementation contract can be reclaimed.

[L-04] The maintainer fee model can be changed in the Factory

The maintainer fee model can be changed in the Factory for new MagicLPs deployed.

Factory.sol#L105-L112

    function setMaintainerFeeRateModel(IFeeRateModel maintainerFeeRateModel_) external onlyOwner {
        if (address(maintainerFeeRateModel_) == address(0)) {
            revert ErrZeroAddress();
        }

        maintainerFeeRateModel = maintainerFeeRateModel_;
        emit LogSetMaintainerFeeRateModel(maintainerFeeRateModel_);
    }

However, this affects new pools deployed and old pools will still use the old maintainer fee model leading to different pools potentially having different fee models which breaks consistency. The maintainer fee rate model contract already has a function to change the rate model implementation and therefore such a function in the factory isn't required.

FeeRateModel.sol#L57-L60

    /// @notice Set the fee rate implementation and default fee rate
    /// @param implementation_ The address of the fee rate implementation, use address(0) to disable
    function setImplementation(address implementation_) public onlyOwner {
        implementation = implementation_;
        emit LogImplementationChanged(implementation_);
    }

[L-05] MagicLP is neither pausable not upgradeable.

MagicLP is neither pausable nor upgradeable (it uses non-upgradeable clones). We don't believe this to be a good idea because if a pool-draining exploit was found it would be nearly impossible to protect all the funds still present in the MagicLP.

[L-06] Router does not verify whether LP provided is a MagicLP

For all swap functions in the Router, it doesn't validate that the lp address provided is an actual MagicLP. While we couldn't find a way to exploit this. Historically, this has been the cause of famous router hacks such as the famous Sushiswap hack - https://maxwelldulin.com/BlogPost/sushiswap-exploit-explained-2023

[L-07] MagicLpAggregator can't work with tokens with more than 18 decimals or if any token oracle returns more than 18 decimals

It will revert if any token oracle returns more than 18 decimals due to underflow in WAD - baseOracle.decimals() or WAD - quoteOracle.decimals().

MagicLpAggregator.sol#L38-L39

        uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
        uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));

It can also revert for tokens with more than 18 decimals due to underflow in (WAD - baseDecimals) or (WAD - quoteDecimals).

MagicLpAggregator.sol#L43-L44

        baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
        quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));

[R-01] Consider a clearer naming for the maximum boost multiplier that can be set in basis points

The BIPS variable caps the maximum boost multiplier at 100%. It could do well to be renamed to MAX_BIPS for better code readability.

• (LockingMultiRewards.sol#L78)[https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/staking/LockingMultiRewards.sol#L78]

- uint256 private constant BIPS = 10_000;
+ uint256 private constant MAX_BIPS = 10_000;

[N-01] Typo in the _udpateUserRewards function naming

As can be seen from the title of this log and in the codebase, the function to update user rewards has a typo and should be corrected.

• (LockingMultiRewards.sol#L536-L539)[https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/staking/LockingMultiRewards.sol#L536-L539]

function _udpateUserRewards(address user_, uint256 balance_, address token_, uint256 rewardPerToken_) internal {
    rewards[user_][token_] = _earned(user_, balance_, token_, rewardPerToken_);
    userRewardPerTokenPaid[user_][token_] = rewardPerToken_;
}

- function _udpateUserRewards(address user_, uint256 balance_, address token_, uint256 rewardPerToken_) internal {
+ function _updateUserRewards(address user_, uint256 balance_, address token_, uint256 rewardPerToken_) internal {
    rewards[user_][token_] = _earned(user_, balance_, token_, rewardPerToken_);
    userRewardPerTokenPaid[user_][token_] = rewardPerToken_;
}