metaverse-user-dao-addendum III

Boolean variables can be used directly and do not need to be compared to true or false.

In the `MUDUniswapV3Staker` contract, function `unstake()` violates the Check-Effects-Interactions pattern. This pattern is a best practice for writing secure smart contracts that involves performing all state changes before making any external function calls. ### External call(s) ```solidity=247 (usdtReleased, mudReleased) = releaseLiquidity(tokenId); ``` ```solidity=335 function releaseLiquidity(uint256 tokenId) private returns (uint256 amount0collected, uint256 amount1collected) { (, , , uint128 liquidity) = getPositionInfo(tokenId); INonfungiblePositionManager.DecreaseLiquidityParams memory params = INonfungiblePositionManager.DecreaseLiquidityParams({ tokenId: tokenId, liquidity: liquidity, amount0Min: 0, amount1Min: 0, deadline: block.timestamp }); nonfungiblePositionManager.decreaseLiquidity(params); (amount0collected, amount1collected) = collectAllFees(tokenId); //return tokens to the owner usdt.transfer(tokenOwner[tokenId], amount0collected); mud.transfer(tokenOwner[tokenId], amount1collected); } ``` ```solidity=316 function collectAllFees(uint256 tokenId) private returns (uint256 amount0, uint256 amount1) { // set amount0Max and amount1Max to uint256.max to collect all fees INonfungiblePositionManager.CollectParams memory params = INonfungiblePositionManager.CollectParams({ tokenId: tokenId, recipient: address(this), amount0Max: type(uint128).max, amount1Max: type(uint128).max }); (amount0, amount1) = nonfungiblePositionManager.collect(params); } ``` ### State variables removed after the call(s) ```solidity=249 delete tokenOwner[tokenId]; ``` An external function call to the `nonfungiblePositionManager.decreaseLiquidity()` and `nonfungiblePositionManager.collect()` functions are made before the state changes are made. This may create a potential vulnerability where an attacker could manipulate the contract's state during the external function call, which could lead to loss of funds or other malicious actions. To prevent such potential issue, it is important to ensure that all external function calls are made after the state changes have been made.

In the `onERC721Received()` function, there is no need to modify the contract state or receive Ether. However, these functions are not explicitly labeled with the `pure` or `view` modifiers. The `pure` modifier indicates that a function does not read or modify the contract's storage or state variables. It is used when a function performs computations based solely on its input parameters and does not have any side effects. The `view` modifier is used for functions that do not modify the contract's state but may read the contract's storage or state variables. It is used when a function provides a read-only view of the contract's data.

In the function `createNewStakingContract()`, there is a `duration` validation check in which the comment mentions "Duration should be 3/6/9 !", but the valid options for the duration variable are actually 3, 6, or 12. The value 9 is not a valid option according to the condition specified in the `require` statement. ```solidity=161 require(duration == 3 || duration == 6 || duration == 12, "Duration should be 3/6/9 !"); ``` In the codebase, there is inconsistent comment regarding the `duration`. ```solidity=85 struct StakingContractInfo { uint256 tokenId; //UNISWAP V3 liquidity NFT position id uint8 duration; //staking period, 3,6,12 in months(every months 30 days) uint startTime;//staking start block time uint endTime;//staking end time uint256 startMUD; //initial MUD staked uint256 startUSDT; //initial USDT staked } ``` ```solidity=138 // Parameters: // usdtToStake: USDT to be staked // mudToStake: MUD to be staked // duration: stake period (3/6/9 months, each month 30 days) // nodeId: DAPP to use, must return in events ```

In the contract `unstakeForMainnetMapping()`, the `contractInfoArray` variable is declared as `storage`, but it is only used for read-only purposes. When a variable is declared as `storage`, any modifications made to it will directly update the state of the contract storage. However, in this function, the `contractInfoArray` is only being read from, and no modifications are being made to it. Declaring a variable as storage is useful when modifying the variable and having those changes persist in the contract's storage. However, if the function just needs to read the data from the variable and doesn't intend to modify it, declaring it as `storage` incurs unnecessary gas costs. ```solidity=361 StakingContractInfo[] storage contractInfoArray = stakingContractInfoMap[addr]; ``` Similarly, the same code exists in the function `getContractInfoByTokenId()`. ```solidity=448 StakingContractInfo[] storage contractInfoArray = stakingContractInfoMap[addressToCheck]; ```

The `createNewStakingContract()` function does not include a check to verify if the `usdtToStake` and `mudToStake` inputs are not zero. This can lead to unintended behavior and potential issues if either of these inputs is mistakenly set to zero. ```solidity=150 function createNewStakingContract(uint256 usdtToStake, uint256 mudToStake, uint8 duration, uint nodeId) external returns ( uint256 tokenId, uint128 liquidity, uint256 amount0, uint256 amount1, uint startTime, uint endTime ) { require(duration == 3 || duration == 6 || duration == 12, "Duration should be 3/6/9 !"); uint256 amount0ToMint = usdtToStake; uint256 amount1ToMint = mudToStake; ```

In the `createNewStakingContract()` function, both `amount0Min` and `amount1Min` are specified with a value of zero when calling the `mint()` function of the `nonfungiblePositionManager`. These parameters are used to enforce price slippage checks in the DEX pair. By setting them to zero, there is no explicit requirement for a minimum amount of tokens to be received in return for the staking transaction. This omission can lead to potential issues with price slippage and unexpected outcomes for the staking operation. ```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); ``` Additionally, the same issue also exists in `prematureUnstake()` and `releaseLiquidity()` functions. ```solidity=288 INonfungiblePositionManager.DecreaseLiquidityParams memory params = INonfungiblePositionManager.DecreaseLiquidityParams({ tokenId: tokenId, liquidity: liquidityOff, amount0Min: 0, amount1Min: 0, deadline: block.timestamp }); (amount0, amount1) = nonfungiblePositionManager.decreaseLiquidity(params); ``` ```solidity=338 INonfungiblePositionManager.DecreaseLiquidityParams memory params = INonfungiblePositionManager.DecreaseLiquidityParams({ tokenId: tokenId, liquidity: liquidity, amount0Min: 0, amount1Min: 0, deadline: block.timestamp }); nonfungiblePositionManager.decreaseLiquidity(params); ``` Therefore, it could be vulnerable to sandwich attacks.

There is a potential issue in the function `unstakeForMainnetMapping()` which uses a loop to iterate over an array, and could potentially result in a high amount of gas consumption. If the array contains an exceedingly large number of elements, the loop may consume more gas than is available in a single block. This would cause the transaction to fail with the "out of gas" error. ```solidity=364 for (uint i=0; i < contractInfoArray.length; i++) { uint256 tokenId = contractInfoArray[i].tokenId; require(tokenOwner[tokenId] == addr, "The addr is not the owner of the token !"); (uint256 usdtReleased, uint256 mudReleased) = releaseLiquidity(tokenId); delete tokenOwner[tokenId]; //delete the nft position from owner map emit mudLPUnstaked(addr, tokenId, usdtReleased, mudReleased); } ```

The `createNewStakingContract()` function allows users to specify arbitrary values for the `nodeId` parameter. However, since the function is public, there is a risk that users may provide unexpected or incorrect values for `nodeId`. This may result in events not being processed correctly by the intended DAPP, potentially causing issues with the expected functionality and data processing.

The linked `transfer()`/`transferFrom()` invocations do not check the return value of the function call, which should yield `true` in the case of a proper `ERC-20` implementation.