Maia DAO Ecosystem - Udsen's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesPool.sol#L252-L270

Vulnerability details

Impact

The UlyssesPool.setWeight function is used to update the weight of a particular poolId in the current UlyssesPool. But when the weight of the respective poolId is decreased from the current value, the calculation underflows and thus DoS the reducing of weight for any poolId in the Ulysses AMM.

This could lead to owner not being able to reduce the weight of a respective poolId based on the required allcoation of current pool's assets for the other pools.

Consider the following scenario:

1. There are three pools in the Ulysses AMM.

2. Out of the three pools, pool1 has two BandwidthStates created by pool2 and pool3 respectively.

3. pool2 has a weight of 30 in the pool1.

4. pool3 has a weight of 70 in the pool1.

5. The owner of the UlyssesPool decides to change the weight of the pool2 to 20 thus reducing the newTotalWeights.

6. Since now the oldTotalWeights > newTotalWeights is true, function will attempt to recalculate the new bandwidths of the respective UlyssesPools as follows:

    if (oldTotalWeights > newTotalWeights) {
        for (uint256 i = 1; i < bandwidthStateList.length;) {
            if (i != poolIndex) {
                uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
                if (oldBandwidth > 0) {
                    bandwidthStateList[i].bandwidth =
                        oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();
   
                    leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
                }
            }
   
            ...
   
    }
   

7. But now the leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth; operation will underflow since the calculated bandwidthStateList[i].bandwidth > oldBandwidth, since oldTotalWeights > newTotalWeights.

8. As a result this will DoS a key component (reducing the weight of a respective PoolId by using the UlyssesPool.setWeight function) of the Ulysses AMM.

Proof of Concept

Add the following testFail_SetWeight test to the UlyssesPoolTest.t.sol test file.

    function testFail_SetWeight() public {
        setUpHandler();

        vm.startPrank(handler);

        UlyssesPool[] memory pools = createPools(3);
        UlyssesPool pool1 = UlyssesPool(pools[0]);
        UlyssesPool pool2 = UlyssesPool(pools[1]);
        UlyssesPool pool3 = UlyssesPool(pools[2]);

        pool1.addNewBandwidth(2, 20); //@audit-info - 
        pool1.addNewBandwidth(3, 80);

        pool2.addNewBandwidth(1, 100);

        pool3.addNewBandwidth(1, 100);

        vm.stopPrank();    

        MockERC20(pool1.asset()).mint(address(this), type(uint256).max);
        MockERC20(pool1.asset()).approve(address(pool1), type(uint256).max);

        MockERC20(pool2.asset()).mint(address(this), type(uint256).max);
        MockERC20(pool2.asset()).approve(address(pool2), type(uint256).max);

        MockERC20(pool3.asset()).mint(address(this), type(uint256).max);
        MockERC20(pool3.asset()).approve(address(pool3), type(uint256).max);

        pool1.deposit(1000 ether, address(this));

        vm.prank(handler);
        pool1.setWeight(2, 10);
        vm.expectRevert("Arithmetic over/underflow");

    }    

Following is the code snippet in the UlyssesPool.sol which has the vulnerability

        if (oldTotalWeights > newTotalWeights) {
            for (uint256 i = 1; i < bandwidthStateList.length;) {
                if (i != poolIndex) {
                    uint256 oldBandwidth = bandwidthStateList[i].bandwidth;
                    if (oldBandwidth > 0) {
                        bandwidthStateList[i].bandwidth =
                            oldBandwidth.mulDivUp(oldTotalWeights, newTotalWeights).toUint248();

                        leftOverBandwidth += oldBandwidth - bandwidthStateList[i].bandwidth;
                    }
                }

                unchecked {
                    ++i;
                }
            }

            poolState.bandwidth += leftOverBandwidth.toUint248();
        } else {

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesPool.sol#L252-L270

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

It is recommended to change the leftOVerBandwidth calculation as below to prevent the underflow of the calculation.

leftOverBandwidth += bandwidthStateList[i].bandwidth - oldBandwidth;

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The UniswapV3Staker.restakeToken() function is used to restake the liquidity NFT every week by the users since the Incentives are weekly. The restakeToken function checks whether the tokenId is already staked, if so it will unstake first and then will retrieve the latest position info of the tokenId and will restake again.

The unstake of the tokenId happens as follows:

    if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);

Once the _unstakeToken() function is called, if the msg.sender == owner then it will allow the unstaking of the tokenId given subsequent conditions are also satisfied. But as per the Natspec comments of the function, if the block time has passed the incentive end time (block.timestamp >= endtime) anyone (not necessarily the owner) can call the restake function.

    // anyone can call restakeToken if the block time is after the end time of the incentive

But this is not true. Since inside the UniswapV3Staker.restakeToken() function, when calling the _unstakeToken function the isNotRestake is set to true. Which means irrespective of the block.timestamp is less or more than the endtime the following condition will result in true since it is logical or operation.

if (isNotRestake || block.timestamp < endTime)

As a result even after the incentive end time has passed only the owner of the tokenId will be able to call the UniswapV3Staker.restakeToken() function, contrary to what the protocol needs to achieve as mentioned in its comments.

As a result if the owner forgets to restake his NFT after one week he will miss out on staking rewards since nobody else can restake on his behalf.

Proof of Concept

        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);

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

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

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

In order to achieve the intended behaviour of the protocol as far as restaking is concerned, it recommended to make the following change in the restake() function as shown below:

    if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false);

Here the isNotRestake is set to false. Now the restaking can be controlled by the block.timestamp and incentive endTime when msg.sender != owner. Hence now anyone can call the restake function once the incentive endTime has elapsed.

Assessed type

Other

1. VAULT ACCEPTS LIMITED NUMBER OF ASSETS

In the ERC4626MultiToken.constructor() following require statement is performed to make sure only assets with 18 decimal places are accepted as deposits to the vault. There are multiple ERC20 assets which are not 18 decimal numbers. Hence the protocol vault limits the number of assets that can be deposited into the vault thus limiting the user experience.

     require(ERC20(_assets[i]).decimals() == 18);

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/ERC4626MultiToken.sol#L51

2. assetsAmounts PASSED IN COULD BE OUT OF ORDER IN COMPARISON TO assets ARRAY

In the ERC4626MultiToken.convertToShares() function, assetsAmounts are used to calculate the shares to mint. Here the assetsAmounts are expected to be passed in the order of the assets array. If there is any misconfiguration then it will effect the internal shares calculation since the weights[i] would be different for the respective assetsAmounts[i]. Hence the minted share amount could be errorneous which could impact the overall accounting of the protocol.

Hence it is recommended to implmenet a validation to verfiy that the assetsAmounts are in the correct order of the assets array. Can be done by passing in a mapping which includes the assetId for the respective assetAmount when calling the ERC4626MultiToken.deposit() function.

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/ERC4626MultiToken.sol#L202 https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/ERC4626MultiToken.sol#L69

3. WHEN EMITTING EVENTS INCLDUE BOTH OLD AND NEW VALUES DURING CRITICAL PARAMETER CHANGES

    emit FlywheelBoosterUpdate(address(newBooster));

In the above it is recommended inclued the old booster address as well in the event to emitted.

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/base/FlywheelCore.sol#L140 https://github.com/code-423n4/2023-05-maia/blob/main/src/gauges/UniswapV3Gauge.sol#L65 https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L548 https://github.com/code-423n4/2023-05-maia/blob/main/src/uni-v3-staker/UniswapV3Staker.sol#L558

4. CRITICAL STATE CHANGES SHOULD EMIT events.

The BaseV2Minter contract has three setter functions to update three state variables (dao, daoShare, tailEmission). But none of the above setter functions emit events. It is recommended to emit events for critical state parameter changes at the end of the respective functions.

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/base/FlywheelCore.sol#L88 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/base/FlywheelCore.sol#L94 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/base/FlywheelCore.sol#L100

5. require STATEMENTS SHOULD HAVE DESCRIPTIVE REVERT MESSAGES

Following instance there is no descriptive revert message in the require statement.

    require(queuedRewards.storedCycle < currentCycle);

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L182

6. USE require INPLACE OF assert STATEMENT.

Prior to solidity version 0.8.0, hitting an assert consumes the ** remainder of the transaction's available gas** rather than returning it, as require() / revert() do. assert() should be avoided even past solidity version 0.8.0 as itsdocumentation states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix".

assert statement is used for invariants which are always true. Hence in the smart contract it is recommened to replace the assert with the require statement.

    assert(queuedRewards.storedCycle == 0 || queuedRewards.storedCycle >= lastCycle);

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L183 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L215

7. THERE IS NO address(0) CHECK FOR to ADDRESS

The UniswapV3Staker.claimReward and UniswapV3Staker.claimRewards function are used to let the users claim thier respective rewards. Here the users are allowed to pass in to address to which the rewards will be transferred. But there is no input validation for the to address. But there is input validation for the amountRequested != 0.

If user mistakenly passes in default value for the to address, his rewards will be sent to the address(0), as a result the rewards will be permenantly lost. Hence it is recommended to perform input validation of address(0) for the to address.

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

8. TYPOS IN THE COMMENTS

// Own Scope to avoid stack `to` deep
This should be corrected as follows:
// Own Scope to avoid stack `too` deep

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

/// @notice Transfer balance of token to rewards contract
This should be corrected as follows:
/// @notice Transfer balance of token to rewards `manager` contract

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/depots/RewardsDepot.sol#L18

/// @notice calculate and transfer accrued rewards to `flywheel core`
This should be corrected as follows:
/// @notice calculate and transfer accrued rewards to `flywheel rewards manager`

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelBribeRewards.sol#L31

/// @dev Update minter cycle and queue `rewars` if needed.   
This should be corrected as follows:
/// @dev Update minter cycle and queue `rewards` if needed.  

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L73 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L108 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L201

// include `uncompleted` cycle    
This should be corrected as follows:
// include `incompleted` cycle

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L92

9. INITIALIZATION TO DEFUALT VALUE CAN BE OMMITTED.

bool private stakeFlag = false;

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

10. BOOLEAN RETURN VALUE OF transfer AND transferFrom IS NOT CHECKED.

It is recommended to check for the return value and revert if the return value is false or else use safeTransfer and safeTransferFrom, from the openzeppelin safeERC20.sol library.

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

11. address(0) CHECK IS NOT PERFORMED FOR THE CRITICAL INPUT PARAMETERS IN THE CONSTRUCTOR

check for address(_bHermesGauges) != address(0) else in the worse case scenario could lead to redeployment (if the _bHermesGauges is uninitialized).

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/booster/FlywheelBoosterGaugeWeight.sol#L48-L50 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L55 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/rewards/FlywheelGaugeRewards.sol#L62 https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/depots/MultiRewardsDepot.sol#L30 https://github.com/code-423n4/2023-05-maia/blob/main/src/hermes/minters/BaseV2Minter.sol#L80