metaverse-user-dao-addendum III

In the contract `MUDUniswapV3Staker` the role `admin` has authority over the functions shown in the list and diagram below. * unstakeForMainnetMapping() - This function unstakes all liquidity NFT positions associated with a specified address, regardless of the contract's expiry time. * getContractCount() - The admin role can use this function to retrieve the total number of staking contracts associated with a specific address. * getContractInfo() - With this function, the admin role can obtain information about a particular staking contract. * getContractInfoByTokenId() - The admin role has the authority to retrieve information about a specific staking contract using the tokenId (liquidity NFT position ID). Any compromise to the `admin` account may allow the hacker to take advantage of this authority. They could potentially query the staking information of any account, which should be considered sensitive and only accessible to the staking owner. Furthermore, the hacker could also unstake the liquidity NFTs associated with any account. 

The team introduced a slippage mechanism in the commit [35ee42f3d5f86ca6abe4e66dcc80b00e7e71da93](https://github.com/MetaUserDAO/MUDUniswapV3Staker/tree/35ee42f3d5f86ca6abe4e66dcc80b00e7e71da93). This mechanism incorporates the `slippageFactor` parameter to ensure that the minimum liquidity tokens added surpass set thresholds, dictated by `usdtToStake` and `mudToStake`. ```solidity function createNewStakingContract(uint256 usdtToStake, uint256 mudToStake, uint8 duration, uint nodeId, uint256 slippageFactor) external returns ( uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1, uint startTime, uint endTime ) ``` The slippage threshold is derived as follows: ```solidity INonfungiblePositionManager.MintParams memory params = INonfungiblePositionManager.MintParams({ token0: usdtAddr, token1: mudAddr, fee: 3000, tickLower: -887220, //this will be adapted to reflect the actual MUD/USDT token pair on the polygon mainnet tickUpper: 887220, //this will be adapted to reflect the actual MUD/USDT token pair on the polygon mainnet amount0Desired: amount0ToMint, amount1Desired: amount1ToMint, amount0Min: SafeMath.mul(amount0ToMint, slippageFactor) / 1000, // slippage safeguard amount1Min: SafeMath.mul(amount1ToMint, slippageFactor) / 1000, // slippage safeguard recipient: address(this), deadline: block.timestamp }); ``` In the `createNewStakingContract()` function, the slippage factor used for calculating the minimum amounts (`amount0Min` and `amount1Min`) required for the liquidity minting is based on the user-provided token amounts (`amount0ToMint` and `amount1ToMint`), rather than the actual amounts needed for the liquidity. This approach can lead to transaction reverts if users stake a large number of `MUD` or `USDT` tokens without knowing the actual proportion between the two tokens. The slippage factor is applied to the user-provided amounts, which may result in unrealistically high minimum amounts. If the actual amounts needed for the liquidity are lower than the slippage-adjusted amounts, the transaction will fail. Additionally, the fix requires users carefully set the `usdtToStake`, `mudToStake` and `slippageFactor`. If those parameters are not properly set, it could still lead to a possible sandwich attack. For example, if the amount of `usdtToStake` and `mudToStake` deviate from the current ratio of usdt:mud in the pool, the threshold that is based on `usdtToStake` and `mudToStake` may be unable to protect from slippage and thus cause the potential loss due to slippage (as the above test case demonstrate).

In the function `createNewStakingContract()`, it passes the fixed values of `tickLower` and `tickUpper` while adding liquidity for a specific trading pair. There doesn't appear to be any immediate issue with the code snippet itself. However, it's important to note that these fixed values may not be suitable for all scenarios. The `tickLower` and `tickUpper` values may need to be adjusted based on the actual MUD/USDT token pair on the Polygon mainnet. It's crucial to ensure that the `tickLower` and `tickUpper` values align with the price range of the actual trading pair. Selecting inappropriate ranges for `tickLower` and `tickUpper` could result in insufficient liquidity provision or an inability to meet the trading demands. ```solidity=174 INonfungiblePositionManager.MintParams memory params = INonfungiblePositionManager.MintParams({ token0: usdtAddr, token1: mudAddr, fee: 3000, tickLower: -887220,//this will be modified to the actual token pair MUD/USDT of polygon mainnet tickUpper: 887220,//this will be modified to the actual token pair MUD/USDT of polygon mainnet amount0Desired: amount0ToMint, amount1Desired: amount1ToMint, amount0Min: 0, amount1Min: 0, recipient: address(this), deadline: block.timestamp }); // Note that the pool defined by MUD/USDT and fee tier 0.3% must already be created and initialized in order to mint (tokenId, liquidity, amount0, amount1) = nonfungiblePositionManager.mint(params); ```

The contract serves as the interface for interacting with the third-party protocol `Uniswap V3`, which involves the utilization of specific addresses associated with the protocol. These addresses include: - `IUniswapV3Pool`: Address related to the `Uniswap V3` pool. - `INonfungiblePositionManager`: Address associated with the non-fungible position manager of `Uniswap V3`. - `IUniswapV3Factory`: Address of the `Uniswap V3` factory. During the audit process, the third-party entities are treated as black boxes, assuming their functional correctness. However, it's important to acknowledge that in real-world scenarios, third-party entities can be compromised, potentially resulting in the loss or theft of assets. In addition to the third-party addresses, there are also addresses involved in the contract's business logic: - `daoFundAddress`: The address of the MUD DAO fund, which receives confiscated USDT. Currently, it is implemented as a Gnosis proxy address. - `usdtAddr`: The address of the USDT token. Currently, it is set as a testing token address. - `mudAddr`: The address of the MUD token deployed on the blockchain. Currently, it is set as a testing token address. It is assumed that these addresses are valid and secure actors, and appropriate logic has been implemented to facilitate collaboration with the current project.

There are some hardcode addresses in this codebase.

There is a hardcode init code hash in `PoolAddress` to calculate the pool address of `token0-token1`. ```solidity=6 bytes32 internal constant POOL_INIT_CODE_HASH = 0xe34f199b19b2b4f47f68442619d555527d244f78a3297ea89325f843f87b8b54; ```

When transferring deflationary ERC20 tokens, it's important to consider that the amount received may not be equal to the amount sent due to transaction fees. For example, if a user sends 100 deflationary tokens with a 10% transaction fee, only 90 tokens may actually arrive at the contract. Failing to account for these fees can lead to issues where a user can withdraw more tokens from the contract than they have actually deposited. In the `MUDUniswapV3Staker` contract, users can stake liqudidity by first transferring tokens to the `MUDUniswapV3Staker` contract. The `MUDUniswapV3Staker` contract could probably transfer the same amount of tokens to `nonfungiblePositionManager` contract. If the pair tokens are deflationary tokens, the function call would potentially revert. ```solidity=166 // transfer tokens to contract usdt.transferFrom(msg.sender, address(this), amount0ToMint); mud.transferFrom(msg.sender, address(this), amount1ToMint); // Approve the position manager usdt.approve(address(nonfungiblePositionManager), amount0ToMint); mud.approve(address(nonfungiblePositionManager), amount1ToMint); INonfungiblePositionManager.MintParams memory params = INonfungiblePositionManager.MintParams({ token0: usdtAddr, token1: mudAddr, fee: 3000, tickLower: -887220,//this will be modified to the actual token pair MUD/USDT of polygon mainnet tickUpper: 887220,//this will be modified to the actual token pair MUD/USDT of polygon mainnet amount0Desired: amount0ToMint, amount1Desired: amount1ToMint, amount0Min: 0, amount1Min: 0, recipient: address(this), deadline: block.timestamp }); // Note that the pool defined by MUD/USDT and fee tier 0.3% must already be created and initialized in order to mint (tokenId, liquidity, amount0, amount1) = nonfungiblePositionManager.mint(params); ```

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.