Unchain X Circular Ecosystem

The function `enqueue()` is designed to schedule an operation with a delay. However, it calls `_validationWithStaticCall()`, which makes the following sequence of static calls ```solidity for (uint256 i = 0; i < data.length; ++i) { (bool success, ) = data[i].target.staticcall(data[i].payload); if (!success) { revert InvalidData(data[i]); } } ``` Similarly `_execute()`, makes the same sequence of calls ```solidity for (uint256 i = 0; i < data.length; ++i) { (bool success, bytes memory returndata) = data[i].target.call(data[i].payload); Address.verifyCallResult(success, returndata); } ``` Both functions aim to execute the provided payloads; however, the static call in `_validationWithStaticCall()` will fail if any state changes occur. This prevents such transactions from being scheduled. Since most payloads should indeed change the blockchain state, this issue is given a *Critical* severity.

The logic of `totalRemainReward()` does not calculate the correct remaining reward due to the following errors in the logic. - The following logic divides the total rewards accumulated for a halving period and divides it by the total share, when it should give the total amount of rewards not the rewards per share. In addition, it should accumulate rewards for the entire halving period, which does not include the block of the halving as this is at the new reward rate. So that `tmpBlock` should be set to `halvingBlocks[i] - 1` and this logic should be executed if `lastBlock >= halvingBlocks[i]`. Not that the incremented amount should be careful to only accumulate rewards for blocks that should have a reward per block given by `rewardPerBlockOf(i)`. ```solidity if(lastBlock > halvingBlocks[i]) { // Calculate reward before halving remains += rewardPerBlockOf(i) * ((halvingBlocks[i] - 1) - tmpBlock + 1) * 1e18 / totalShare; tmpBlock = halvingBlocks[i]; } ``` - The following logic sets the return amount remains to the amount of rewards for since the last halving. However, it should increment by this amount so that it accounts for all rewards. In addition, the logic calls `rewardPerBlock()`, which calls `halvingProtocol.currentRewardPerBlock()` which returns 0 if the current block is greater than the end block. Thus if it is called after the `endBlock` it will add 0 to `remains` and not account for all of the rewards accumulated after the last halving. ```solidity if(tmpBlock <= lastBlock) { remains = rewardPerBlock() * (lastBlock - tmpBlock + 1); } ``` - The following line does not assign the result of the operation to `remains`. As such the amount returned will be the total amount of rewards as opposed to the total amount of remaining rewards. ```solidity remains - claimedReward; ```

The `TimeLockController` contract will not compile and will trigger the compiler error: ```text "Unimplemented feature (/solidity/libsolidity/codegen/ArrayUtils.cpp:227): Copying of type struct IBaseTimeLock.DataParams calldata[] calldata to storage not yet supported." ``` The issue stems from the following line: ```sol=54 _data[id] = params; ``` This line attempts to write a dynamic calldata array to a dynamic storage array, which is currently not supported.

The function `preAllocateVotingPower()` checks if `preVotingPowerAllocation` is true to determine if the pre-allocation of voting power has been given. However, `preVotingPowerAllocation` is not set to true within the `preAllocateVotingPower()` function and there is no method in the contract to update it to true. In addition, `preAllocateVotingPower()` calls ` _updateVotes(params[i].account, 0, params[i].votingPower)` which assumes that users have no current votes. As such it should be ensured that this is called prior to `initialize()` being called otherwise if users have called `addShare()` before, then they will lose their votes obtained from burning tokens.

The `DataParams` struct is defined as ```solidity struct DataParams { address target; bytes payload; } ``` So that each scheduled transaction can only specify the `target` of the call and the `payload` to be sent. If a transaction requires native tokens to be sent via `msg.value`, there is no parameter allowing this value to be specifed. So any transactions requiring native tokens will not be able to be supported. If a transaction requires a previous transaction before it can be executed, then there is no parameter to specify the previous transaction so that it can be checked to be completed. Without these checks it is possible that transactions that should be executed in a specific order may be executed out of order and cause unexpected issues.

The client states that there would be no transactions requiring `msg.value`. As such, we assume that no transaction should have a target be an EOA. However, if the `target` is set to an EOA the call will succeed as a low level call to an EOA will return as successful.

The function `mintVotingToken()` includes a variable `minRequireForMint`, which causes a transaction to revert if a user attempts to mint fewer than this specified minimum amount of `votingToken`. This restriction can result in users having a nonzero, insufficient amount of votes that they are unable to utilize for minting additional voting tokens if `minRequireForMint > 1`. For example, if `minRequireForMint = 2` and a user has `votesOf[caller] = 11`, the user might mistakenly call `mintVotingToken(10)`, successfully minting 10 voting tokens and updating their allocation to `votesOf[caller] = 1`. Consequently, the user will be unable to mint the remaining single vote due to the minimum requirement. Similarly, if a user’s balance is exactly `votesOf[caller] = 1`, they will never be able to mint their allocated vote because it does not meet the `minRequireForMint` threshold.

The **require** can be used to check for conditions and throw an exception if the condition is not met, however, the sited line will revert without giving any error message.

In the `TimeLockController` constructor, the values of `minDelay_` and `maxDelay_` are assigned without validation. We recommend adding checks to ensure that these are within acceptable limits and that `minDelay_ < maxDelay_`. In the `_setAllocation()` function of the contract `XRecycling`, `allocation` will be assigned without any validation. We recommend adding checks to ensure that this value is within an acceptable limit. In particular, the value should never be larger than `10000`, which represents 100%.

The function `claim()` is designed to have a user lose the percentage of shares they have determined by the percentage of the total remaining rewards that they are claiming. The following calculation causes the amount of shares for a users to lose when claiming to be rounded up ```solidity uint256 quotient = totalRemainReward() / requiredReward; uint256 subShare = currentShare / quotient; ``` This can lead to an large loss of shares if a user inputs a large percentage of the `totalRemainReward()`. For example, if `totalRemainReward() = 100e18` and `requiredReward = 51e18`, then `quotient = 1.96078431373` which will round down to 1. So that `subShare = currentShare` and a user would need to lose all their shares to claim this amount, when they are only claiming a little over half of the total rewards. Considering users can protect against this with the `minRequiredRemains` input we give this a *Minor* severity.