The Unfettered

Accessing storage variables in a loop can be costly in terms of gas consumption. ```solidity=404 totalRewardAmount -= tokenRewards[_rewardOwner][i].amount; ``` --- ```solidity=430 totalRewardAmount += tokenRewards[_rewardOwner][i].amount; ```

`tx.origin` is widely discouraged as there are possible phishing attempts. For a contract, the `tx.origin` is not transparent. In a simple call chain A->B->C->D, inside D msg.sender will be C, and tx.origin will be A. If the origin is really desired in D, then each of the functions in the contracts B, C, D could take an extra parameter to propagate the origin: A would pass its address (this) to B, B would pass the value to C, and C would pass it to D. `tx.origin` based protection can be abused by a malicious contract if a legitimate user interacts with the malicious contract. Hence it's not recommended to use `tx.origin` for authorization in solidity documentation. If one of the `manager` roles calls a phishing contract and then the contract calls the `transferFrom` of the `SoulsToken`, the bot prevention mechanism will be invalid. Refer to: https://docs.soliditylang.org/en/v0.7.0/security-considerations.html#tx-origin

It is important to ensure that there are sufficient rewards in the `staking` contract to avoid a situation where stakers may not be able to withdraw their rewards. Additionally, it is worth noting that the `withdrawStake` function withdraws all the staking tokens and rewards, which could result in the inability to withdraw staking tokens if there are insufficient rewards.

Based on the logic of the `isApproved` function, if the number of affirmative approval reaches three, the topic will be considered approved. ```solidity=182 function changeManager1Address(address _newAddress) external onlyManager(msg.sender) { ... bool _isApproved = isApproved(_title, _encodedValues); if (_isApproved) { manager1 = _newAddress; _deleteTopic(string.concat("Managers: ", _title)); } ... } ``` We understand all managers call the function `changeManager1Address` until the approval reaches three, the manager's address will get changed. However, in a specific scenario, there are four managers who have the ability to use the `changeManager1Address` function to modify a manager's address. If only one manager utilizes this function, the manager's address will not be modified because only one approval for the change topic has been received at that point. If the other three managers choose to approve the topic of changing the manager's address by calling the `approveTopic` function instead of using the `changeManager1Address` function, the number of approvals will reach three. Despite this, the manager's address (L196: `manager1 = _newAddress;`) will remain unchanged because there are no additional managers who have the ability to call the `changeManager1Address` function to carry out the change since each manager can only approve a topic once (as seen in `require(!managerApprovalsForTopic[_title][tx.origin].approved, "Already voted");`). ```solidity=324 function _approveTopic(string memory _title, bytes memory _encodedValues) private { ... require(!managerApprovalsForTopic[_title][tx.origin].approved, "Already voted"); ... } ``` We would like to confirm with the client if the current implementation aligns with the original project design.

The below code segment approves the Router contract to transfer a max amount of tokens on behalf of the owner of the tokens. This can be a security risk if the Router contract is compromised or if the owner of the tokens did not intend to approve a max amount. ````solidity IERC20(soulsTokenAddress).approve(address(routerAddress), type(uint256).max); IERC20(stableTokenAddress).approve(address(routerAddress), type(uint256).max); ```` What needs to be reminded is that if the update here is for a specific quantity, then in the `addLiquidityOnDEX` method, re-approval is required.

According to the following code: ```solidity function addToBlacklist(address _rewardOwner, string calldata _description) external onlyManager { ... for (uint256 i = 0; i < tokenRewards[_rewardOwner].length; i++) { if (tokenRewards[_rewardOwner][i].releaseDate > block.timestamp) { _remainingAmount += tokenRewards[_rewardOwner][i].amount; totalRewardAmount -= tokenRewards[_rewardOwner][i].amount; } } if (!soulsToken.transfer(crowdfundingVault, _remainingAmount)) { revert TokenTransferError(); ... } ``` The `addToBlacklist()` function records the invoking time as the `blacklistDate` for a `_rewardOwner`. After the `blacklistDate`, the `_rewardOwner` will not be able to claim the corresponding vesting reward. Additionally, all vesting rewards prior to the `blacklistDate` will be transferred from the `Crowdfunding` contract to the `_crowdfundingVault`. This may cause problems as follows: 1. The `_rewardOwner` may still claim all vesting rewards prior to the `blacklistDate`, even if they have been blacklisted. 2. Some of the vestings may have already been transferred to the `_crowdfundingVault` when they are deactivated. As a result, the reward tokens in the `Crowdfunding` contract may not be sufficient for other users to claim.

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.

Managers can add extra liquidity on DEX via this function, but we noticed no more stable tokens transferred to the liquidityVault. The stable tokens transferred previously have been used up in the initial liquidity activity.

State variables that never change should be declared as `constant` to save gas.

It is recommended to check that the `_activeStakerCount` is not zero before executing the `for` loop.