Metacraft

The deletion logic in the `deleteTransaction` function of the provided Solidity contract can be optimized. Currently, it uses a loop to shift all the elements after a certain index down by one to fill a gap created by deleting an element. This is not gas-efficient, especially if the array `_pendingTransactions` becomes large. ```solidity for(uint i = 0; i < _pendingTransactions.length; i++) { if (1 == replace) { _pendingTransactions[i-1] = _pendingTransactions[i]; } else if (transactionId == _pendingTransactions[i]) { replace = 1; } } delete _pendingTransactions[_pendingTransactions.length - 1]; _pendingTransactions.pop(); ``` Instead of shifting all elements after the deleted index, the last element of the array is moved into the spot of the element we want to remove. This reduces the cost of deleting an element from O(n) to O(1), since we only need to update one index position rather than potentially shifting a large number of elements.

The `MCTPStake` contract includes a mechanism intended to secure updating the withdrawal address, by requiring multiple signatures from designated `owners`. The function `setWithdrawAddress` initiates transaction proposals, while `signTransaction` is used by owners to approve these proposals. ```solidity function signTransaction(uint transactionId) validOwner public { ... require(block.timestamp - transaction.timestamp >= 24 hours,"Time Lockin 48 Hours for at Least 2 signers after Tx created! !"); ... if (transaction.signatureCount >= MIN_SIGNATURES) { withdrawAddress = payable(transaction.withdrawAddress_); emit TransactionCompleted(transaction.withdrawAddress_, block.timestamp, transactionId); deleteTransaction(transactionId); } } ``` The `signTransaction` function requires each signature to occur at least 24 hours apart, totaling a 48-hour period which is designed to be the time window for owners to sign. As soon as the minimum signature threshold is met, the proposed transaction is executed instantly. The timelock period serves as a voting/approval period for owners. However, there is no isolation of the approval period (for signing by the owners) and the execution period (the window during which users can observe the approved transaction before it takes effect). Specifically, once a transaction has obtained the minimum required signatures, it is immediately executed without an additional delay. This design overlooks the need for a separate period during which stakeholders can detect and respond to impactful administrative actions. Without this observation period, users might not be able to make informed decisions, such as withdrawing their stakes in response to changes they disagree with or find alarming.

For ERC-20 tokens, the number of decimals can vary. A common standard is 18 decimals, just like ETH, but some tokens might use fewer. For instance, USDC, a popular stablecoin, uses 6 decimals. ```solidity function openStake( address _itemToken, uint256 _tokenAmount, uint256 _duration ) public whenNotPaused virtual { require(_tokenAmount >= minStakeAmount,"stake tokenAmount too small!"); ... totalStakeAmount+=_tokenAmount; stakeAmounts[_duration]+=_tokenAmount; ... } ``` When staking, the function imposes a minimum stake amount with a decimal precision of 18. This raises the threshold and complicates the utilization of the function for tokens with decimals other than 18, such as USDC. Additionally, the `totalStakeAmount` and `stakeAmounts[_duration]` accumulate the `_tokenAmount` from different `_itemToken`, making the value of these two state variables meaningless since each `_itemToken` may have different decimals.

1. In `executeTransaction()` function. This function can be triggered by any account without owner privilege, as previously mentioned by the team. However, within this function, the usage of `deleteTransaction()` is confined to `validOwner`. Consequently, this constraint means that in practice, only a validOwner is capable of actually carrying out the `executeTransaction()` function, leading to a situation that diverges from the anticipated operational scenario. ```solidity function executeTransaction(uint transactionId) public { ... deleteTransaction(transactionId); } modifier validOwner() { require(_owners[msg.sender] == 1, "not Authorized Mulsig User !"); _; } function deleteTransaction(uint transactionId) validOwner private { ... } ``` 2. In `deleteMaxPendingTx()` function. This function is accessible through owner-controlled (`owner`) operations such as `addOwner()`, `removeOwner()`, and `setWithdrawAddress()`. Within its scope, it carries out the deletion of pending transactions that have exceeded their timeout limit. However, the transaction removal function is exclusively callable by a `validOwner`, implying that only individuals who possess both owner and validOwner statuses can effectively implement the `deleteMaxPendingTx()` function's intended actions. ```solidity function addOwner(address _owner) public { require(msg.sender==owner,"Only owner can set Parameters"); deleteMaxPendingTx(); ... } function deleteMaxPendingTx() private { ... deleteTransaction(txId); ... } modifier validOwner() { require(_owners[msg.sender] == 1, "not Authorized Mulsig User !"); _; } function deleteTransaction(uint transactionId) validOwner private { ... } ```

The logic to remove a transaction attempts to shift all the subsequent elements in `_pendingTransactions` array leftward after finding the specified transactionId. However, it lacks proper validation to determine if the `transactionId` actually exists in the array. If it's not found, the code sets replace to false and will eventually delete the last item of `_pendingTransactions` regardless of whether it relates to the specified `transactionId`. ```solidity function deleteTransaction(uint transactionId) validOwner public { uint8 replace = 0; for(uint i = 0; i < _pendingTransactions.length; i++) { if (1 == replace) { _pendingTransactions[i-1] = _pendingTransactions[i]; } else if (transactionId == _pendingTransactions[i]) { replace = 1; } } delete _pendingTransactions[_pendingTransactions.length - 1]; _pendingTransactions.pop(); delete _transactions[transactionId]; } ```

The `checkVerify` and `splitSignature` functions seem to be functions for testing purpose. According to the official Solidity [documentation](https://docs.soliditylang.org/en/v0.8.18/solidity-by-example.html#id3), the function `splitSignature()` does not use all security checks. A real implementation should use a more rigorously tested library, such as openzepplin’s version of this code.

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. *If the itemToken is a malicious contract, the attacker's profit will only affect the itemToken, making it an inefficient operation for them. However, the reentrancy vulnerability may still cause other issues in the middle of transaction.* Reentrancy may occur in the following contracts because of the malicious itemToken contract: - `openStake()`: Allow users to participate in staking. - `upgradeStake()`: Upgrade stake from lower duration to higher Duration Level.

The team uses a 3 of 3 multi-signature mechanism to weaken the centralization risk of the owner. However, changes in the `validOwner` might lead to operational uncertainties, especially in functions requiring multi-signature consensus. For instance, a transaction pending due to lack of enough approval from all valid owners might suddenly proceed if a new owner, who agrees, is introduced and signs off on it, thereby allowing the transaction to be executed and implemented.

The `signTransaction` function allows owners to sign a transaction for its execution. The condition for a transaction to become ready for execution (`transaction.readyForExecutionTimestamp`) is that it must be signed by at least `MIN_SIGNATURES` number of owners. Once this condition is met, the transaction is considered ready for execution after an observation period (`observationPeriod`, which is 24 hours in this case) has passed since the last signature was added. ```solidity function signTransaction(uint transactionId) validOwner public { ... if (transaction.signatureCount >= MIN_SIGNATURES) { transaction.readyForExecutionTimestamp = block.timestamp + observationPeriod; emit TransactionReadyForExecution(transaction.withdrawAddress_, transaction.readyForExecutionTimestamp, transactionId); } } ``` However, **if additional owners sign the transaction after it has already reached the minimum required signatures (`MIN_SIGNATURES`), the `transaction.readyForExecutionTimestamp` will be updated to 24 hours after the most recent signature**, pushing the execution window further into the future each time an additional signature is added. This behavior can potentially delay the execution of a transaction indefinitely, as long as additional owners continue to sign within each new 24-hour window.

In the `buyNode` function, the caller must provide a `message` parameter which is supposed to match a challenge generated internally within the function using `getChallenge`. This design pattern is typically used to verify that the caller is indeed who they claim to be by validating a signature against a known hash. However, if the `message` is simply a hash of known parameters that are already inputs to the function (`timestamp, nonce, parentAddress, msg.sender`), then passing `message` as an input parameter is redundant. ```solidity function buyNode( uint256 timestamp, bytes32 nonce, bytes32 message, bytes memory signature, address parentAddress, uint256 maxPrice ) nonReentrant notContract external returns (uint256){ require(!halted, "Node Halted!"); bytes32 challenge = getChallenge(timestamp, nonce, parentAddress, msg.sender); require(message == challenge,"message not correct!"); ... } ```