Maia DAO Ecosystem - jasonxiale's results

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchPort.sol#L388-L390 https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1340-L1342

Vulnerability details

Impact

Although this issue will not cause user's lost, but it might work against user's will.

Proof of Concept

BranchPort._denormalizeDecimals is defined as

    function _denormalizeDecimals(uint256 _amount, uint8 _decimals) internal pure returns (uint256) {
        return _decimals == 18 ? _amount : _amount * 1 ether / (10 ** _decimals);
    }

supposed Alice wants to bridgeOut 1$ worth USDC, the _deposit paramter is supposed to be 1* 10**18, and since the decimals is 6
so after calling _denormalizeDecimals, the amount would be 1 * 10**18 * 1 eth / 10**6 which is wrong

similar issue exists in BranchBridgeAgent.sol _normalizeDecimals

Tools Used

Recommended Mitigation Steps

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-amm/UlyssesToken.sol#L61-L85

Vulnerability details

Impact

the weights would be wrong after removing an asset

Proof of Concept

to make the test easier, I made some change in the source code:

1. comment out ERC4626MultiToken.sol#L51

--- a/src/erc-4626/ERC4626MultiToken.sol
+++ b/src/erc-4626/ERC4626MultiToken.sol
@@ -48,7 +48,7 @@ abstract contract ERC4626MultiToken is ERC20, ReentrancyGuard, IERC4626MultiToke
         if (length != _assets.length || length == 0) revert InvalidLength();

         for (uint256 i = 0; i < length;) {
-            require(ERC20(_assets[i]).decimals() == 18);
+            //require(ERC20(_assets[i]).decimals() == 18);
             require(_weights[i] > 0);

             _totalWeights += _weights[i];

2. save the following code in UlyssesTokenMock.sol file, and put the file under ulysses-amm, the difference between UlyssesTokenMock.sol and UlyssesToken.sol is that I remove unnecessary token transfer and updateAssetBalances

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Ownable} from "solady/auth/Ownable.sol";
import {FixedPointMathLib} from "solady/utils/FixedPointMathLib.sol";
import {SafeTransferLib} from "solady/utils/SafeTransferLib.sol";

import {ERC20} from "solmate/tokens/ERC20.sol";

import {ERC4626MultiToken, IERC4626MultiToken} from "@ERC4626/ERC4626MultiToken.sol";

import {IUlyssesToken} from "./interfaces/IUlyssesToken.sol";

/// @title Ulysses Token - tokenized Vault multi asset implementation for Ulysses pools
contract UlyssesTokenMock is ERC4626MultiToken, Ownable, IUlyssesToken {
    using SafeTransferLib for address;
    using FixedPointMathLib for uint256;

    uint256 public immutable id;

    constructor(
        uint256 _id,
        address[] memory _assets,
        uint256[] memory _weights,
        string memory _name,
        string memory _symbol,
        address _owner
    ) ERC4626MultiToken(_assets, _weights, _name, _symbol) {
        _initializeOwner(_owner);
        require(_id != 0);
        id = _id;
    }

    /*//////////////////////////////////////////////////////////////
                            ACCOUNTING LOGIC
    //////////////////////////////////////////////////////////////*/

    /// @inheritdoc IERC4626MultiToken
    function totalAssets() public view override returns (uint256 _totalAssets) {
        return totalSupply;
    }

    ///@inheritdoc IUlyssesToken
    function addAsset(address asset, uint256 _weight) external nonReentrant onlyOwner {
        if (assetId[asset] != 0) revert AssetAlreadyAdded();
        //require(ERC20(asset).decimals() == 18);
        require(_weight > 0);

        assetId[asset] = assets.length + 1;
        assets.push(asset);
        weights.push(_weight);
        totalWeights += _weight;

        emit AssetAdded(asset, _weight);

        //updateAssetBalances();
    }

    ///@inheritdoc IUlyssesToken
    function removeAsset(address asset) external nonReentrant onlyOwner {
        // No need to check if index is 0, it will underflow and revert if it is 0
        uint256 assetIndex = assetId[asset] - 1;

        uint256 newAssetsLength = assets.length - 1;

        if (newAssetsLength == 0) revert CannotRemoveLastAsset();

        totalWeights -= weights[assetIndex];

        address lastAsset = assets[newAssetsLength];

        assetId[lastAsset] = assetIndex;
        assets[assetIndex] = lastAsset;
        weights[assetIndex] = weights[newAssetsLength];

        assets.pop();
        weights.pop();
        assetId[asset] = 0;

        emit AssetRemoved(asset);

        //updateAssetBalances();

        //asset.safeTransfer(msg.sender, asset.balanceOf(address(this)));
    }

    ///@inheritdoc IUlyssesToken
    function setWeights(uint256[] memory _weights) external nonReentrant onlyOwner {
        if (_weights.length != assets.length) revert InvalidWeightsLength();

        weights = _weights;

        uint256 newTotalWeights;

        for (uint256 i = 0; i < assets.length; i++) {
            newTotalWeights += _weights[i];

            emit AssetRemoved(assets[i]);
            emit AssetAdded(assets[i], _weights[i]);
        }

        totalWeights = newTotalWeights;

        updateAssetBalances();
    }

    /**
     * @notice Update the balances of the underlying assets.
     */
    function updateAssetBalances() internal {
        for (uint256 i = 0; i < assets.length; i++) {
            uint256 assetBalance = assets[i].balanceOf(address(this));
            uint256 newAssetBalance = totalSupply.mulDivUp(weights[i], totalWeights);

            if (assetBalance > newAssetBalance) {
                assets[i].safeTransfer(msg.sender, assetBalance - newAssetBalance);
            } else {
                assets[i].safeTransferFrom(msg.sender, address(this), newAssetBalance - assetBalance);
            }
        }
    }
}

3. save the following code in UlyssesTokenTestMock.t.sol file, and put the file under test/ulysses-amm/UlyssesTokenTestMock.t.sol

// SPDX-License-Identifier: AGPL-3.0
pragma solidity >=0.8.0 <0.9.0;

import {UlyssesTokenMock} from "@ulysses-amm/UlyssesTokenMock.sol";

import {DSTestPlus} from "solmate/test/utils/DSTestPlus.sol";
import {console2} from "forge-std/console2.sol";
contract InvariantUlyssesTokenMock is DSTestPlus{
    address public _vault;

    function setUp() public {
        address[] memory _assets;
        uint256[] memory _weights;

        _assets = new address[](1);
        _assets[0] = address(uint160(0x1000));
        _weights = new uint256[](1);
        _weights[0] = 1;

        _vault = address(new UlyssesTokenMock(1, _assets, _weights, "Mock ERC4626", "MERC4626", address(this)));
    }

    function testMockAddRemoveAsset() public {
        for(uint i = 1; i < 0x10; i++) {
            UlyssesTokenMock(_vault).addAsset(address(uint160(i + 0x1000)), i + 1);
        }

        /*
            Here, the asset and weight mapping shuld be:
            assets                 weight
            0x1000                 1
            0x1001                 2
            0x1002                 3
            0x1003                 4
            0x1004                 5
            0x1005                 6
            0x1006                 7
            0x1007                 8
            0x1008                 9
            0x1009                 10
            0x100a                 11
            0x100b                 12
            0x100c                 13
            0x100d                 14
            0x100e                 15
            0x100f                 16
        */

        // 136 should be the total weights as 1 + 2 + 3 + .. +16 = 136
        UlyssesTokenMock(_vault).removeAsset(address(0x1001));
        assertEq(136 - 2 , UlyssesTokenMock(_vault).totalWeights());
        UlyssesTokenMock(_vault).removeAsset(address(0x100f));
        assertEq(136 - 2 - 16, UlyssesTokenMock(_vault).totalWeights());
    }
}

4. run the test forge test --mc InvariantUlyssesTokenMock --mt testMockAddRemoveAsset -vv and the function revert

Tools Used

VS

Recommended Mitigation Steps

please consider using the follow change in UlyssesToken.removeAsset

@@ -69,7 +69,7 @@

         address lastAsset = assets[newAssetsLength];
 
-        assetId[lastAsset] = assetIndex;
+        assetId[lastAsset] = assetIndex + 1;
         assets[assetIndex] = lastAsset;
         weights[assetIndex] = weights[newAssetsLength];

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/BranchBridgeAgent.sol#L1349-L1357

Vulnerability details

Impact

BranchBridgeAgent._normalizeDecimalsMultiple will always revert because of lacking of allocating memory

Proof of Concept

BranchBridgeAgent._normalizeDecimalsMultiple's code as below, because deposits is never allocated memory, the function will always revert

    function _normalizeDecimalsMultiple(uint256[] memory _deposits, address[] memory _tokens)
        internal
        view
        returns (uint256[] memory deposits)
    {
        for (uint256 i = 0; i < _deposits.length; i++) {
            deposits[i] = _normalizeDecimals(_deposits[i], ERC20(_tokens[i]).decimals());
        }
    }

Tools Used

VS

Recommended Mitigation Steps

@@ -1351,7 +1351,9 @@
         view
         returns (uint256[] memory deposits)
     {
-        for (uint256 i = 0; i < _deposits.length; i++) {
+        uint len = _deposits.length;
+        deposits = new uint256[](len);
+        for (uint256 i = 0; i < len; i++) {
             deposits[i] = _normalizeDecimals(_deposits[i], ERC20(_tokens[i]).decimals());
         }
     }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L409-L410

Vulnerability details

Impact

ERC20 standard allows transferF function of some contracts to return bool or return nothing. Some tokens such as USDT return nothing. This could lead to funds stuck in the contract without possibility to retrieve them. Using safeTransferFrom of SafeERC20.sol is recommended instead.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/talos/base/TalosBaseStrategy.sol#L409-L410

Tools Used

vs

Recommended Mitigation Steps

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

deposit owner might lost some rewards.

Proof of Concept

According to comments, UniswapV3Staker.restakeToken is supposed to be called by anyone if the block time is after the end time of the incentive. However due to wrong value as isNotRestake of function _unstakeToken in UniswapV3Staker.sol#L342, UniswapV3Staker.restakeToken can only be called by deposit.owner.

UniswapV3Staker.restakeToken calls UniswapV3Staker._unstakeToken which true as isNotRestake

    function restakeToken(uint256 tokenId) external {
        IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true); <-- here isNotRestake is true

        (IUniswapV3Pool pool, int24 tickLower, int24 tickUpper, uint128 liquidity) =
            NFTPositionInfo.getPositionInfo(factory, nonfungiblePositionManager, tokenId);

        _stakeToken(tokenId, pool, tickLower, tickUpper, liquidity);
    }

And according (https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/uni-v3-staker/UniswapV3Staker.sol#L374), if isNotRestake is true and owner != msg.sender, the function will revert no matter if block.timestamp < endTime

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

Tools Used

VS

Recommended Mitigation Steps

     function restakeToken(uint256 tokenId) external {
         IncentiveKey storage incentiveId = stakedIncentiveKey[tokenId];
-        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, true);
+        if (incentiveId.startTime != 0) _unstakeToken(incentiveId, tokenId, false);
 
         (IUniswapV3Pool pool, int24 tickLower, int24 tickUpper, uint128 liquidity) =
             NFTPositionInfo.getPositionInfo(factory, nonfungiblePositionManager, tokenId);

Assessed type

Access Control