HanIdentity - Addendum

In the contract `HanTimelockController` the role `_owner` has authority over the functions shown below: - recoverERC20(): withdraw any ERC20 tokens from the contract to the `owner` account. - recoverERC721(): withdraw any ERC721 tokens from the contract to the `owner` account. Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and do the following: * Withdraw all ERC20 and ERC721 assets from the contract. In the contract `TokenVestingLock` the role `_owner` has authority over the functions shown below: - recoverERC20(): withdraw any ERC20 tokens from the contract to the `owner` account. If the token is the release token, the unreleased portion will be kept in the contract. - recoverERC721(): withdraw any ERC721 tokens from the contract to the `owner` account. Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and do the following: * Withdraw ERC20 and ERC721 assets from the contract.

Based on the linked code, the deployer of the contract `HanTimelockController` will be granted the admin role. With this role, the deployer could influence the time lock deeply. This cause the time lock to lose its meaning. An optional admin can assist with the initial configuration of the role after deployment without delay, but this role should then be dropped to support administration through time locked proposals. ```solidity constructor( uint256 minDelay, address[] memory proposers, address[] memory executors ) TimelockController(minDelay, proposers, executors, msg.sender) {} ```

In the contract `HanTimelockController` the role `_owner` has authority over the functions shown below: - recoverERC20() - recoverERC721() Assuming a proposal requires the contract `TimeLockController` to act as a temporary fund intermediary and the proposal is approved. At some point, the contract `TimeLockController` may hold a significant amount of funds. If the owner's account private key is compromised, a hacker could withdraw all ERC20 and ERC721 assets from the contract, resulting in substantial losses. From a design perspective, since the contract does not have a need to withdraw funds, we consider the mentioned centralized functions to be irrelevant.

Mathematical operations in the aforementioned lines perform divisions before multiplications. Performing multiplication before division can sometimes avoid loss of precision. ```solidity=81 uint256 tokensPerRoundPerBeneficiary = totalReleaseTokens * intervalSeconds / durationSeconds * _shares[i] / 100; ```

According to the logic of the function `recoverERC20()`, `_tokenAmount` tokens will be transferred to the owner. However, the value recorded in the event is the maximum token amount that can be recovered but not the real amount that was transferred. ```solidity IERC20(_tokenAddress).transfer(msg.sender, _tokenAmount); emit RecoveredERC20(_tokenAddress, tokenAmount); ```

According to the design, `durationSeconds` represents the duration of the vesting period in seconds, `intervalSeconds` represents the duration of each round in seconds. The user's earnings are based on the following calculation formula: ```solidity=81 uint256 tokensPerRoundPerBeneficiary = totalReleaseTokens * intervalSeconds / durationSeconds * _shares[i] / 100; uint256 releaseTokens = tokensPerRoundPerBeneficiary * totalRounds; ``` However, the function does not verify whether `durationSeconds` can be evenly divided by `intervalSeconds`. If it cannot, the result calculated based on the above formula will not match the actual outcome.

According to the formula for the local variable `nextIntervalTime`, its value is a time period, most likely less than the current time. Therefore, the linked `if` judgement will always return true and is therefore meaningless. ```solidity= uint256 nextIntervalTime = (currentInterval + 1) * intervalSeconds; if (nextIntervalTime <= currentTime) { ```

In #L110, the local variable `unreleased` was assigned with amount of tokens released so far. The `for` loop below will release all tokens after the last release. Therefore, deducting `tokensToRelease` this time from `unreleased` is meaningless since `unreleased` contains the amount that had been released before. The value of `unreleased` is greater than 0 definitely. Even in the case that release all tokens at once, it is still meaningless because the sum of the released tokens is smaller than or equal to the whole amount will be released. ```solidity uint256 unreleased = totalVestedTokens; for (uint256 i = 0; i < payees.length; i++) { uint256 payeeShare = (payees[i].shares * totalVestedTokens) / 100; uint256 releasable = payeeShare - releasedAmount[payees[i].account]; if (unreleased > 0 && releasable > 0) { uint256 tokensToRelease = (releasable < unreleased) ? releasable : unreleased; releasedAmount[payees[i].account] += tokensToRelease; unreleased -= tokensToRelease; totalReleasedTokens += tokensToRelease; token.transfer(payees[i].account, tokensToRelease); emit released(payees[i].account, tokensToRelease); } } ```

According to the comments above the function `constructor()`, the `accounts` cannot contains duplicate account. ```solidity= /* All addresses in `accounts` must be non-zero. Both arrays must have the same non-zero length, and there must be no * duplicates in `accounts` */ ```

The linked variables assigned in the constructor can be declared as `immutable`. Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.