Omnia Protocol security record

The `emergencyWithdraw()` function in the `Staking` contract permits the admin (via a time-lock contract) to extract any chosen amount of tokens from the contract, including those deposited by users. ```solidity=634 function emergencyWithdraw(uint256 amount, bytes32 reason) external whenPaused onlyAdmin { require(amount > 0, "Cannot be zero"); IERC20(_token).safeTransfer(msg.sender, amount); emit EmergencyTokenWithdraw(msg.sender, reason, amount); } ``` This function can only be activated when the staking contract is paused and enables the admin to remove tokens from the staking contract. There is a significant risk associated with this function, as it could lead to the loss of all staked funds by investors, even if the nodes they have staked in are operating correctly. This could occur if the function is misused.

As a best practice, the `nonReentrant` modifier could be placed and executed before other modifiers in functions to prevent reentrancy attacks through other modifiers and make the code more efficient.

The `unstakeTokens()` function permits users to retrieve their stakeAmount. The `_stakes[stakeId].amount` variable denotes the amount staked by the user in stakeId. Nonetheless, at the end of the function, the contract fails to update the variable following the transfer of stakeAmount to the user. ```solidity function unstakeTokens(uint256 stakeId) external nonReentrant { Stake storage stake = _stakes[stakeId]; ... _nodes[stakeNodeId].stakedAmount -= stakeAmount; IERC20(_token).safeTransfer(msg.sender, claimReward > 0 ? _sumUintInt(stakeAmount, claimReward) : stakeAmount); ... } ```

The codebase contains several `TODO` comments indicating unfinished tasks or features that require further development or revision. The presence of these comments makes it challenging to ascertain the completeness and current status of the implicated sections.

The `_claim()` function, being of private type, is tasked with determining the staking rewards a user is eligible for. This calculation initiates from the `stakeStartTimestamp`, incorporating both the first epoch's partial reward and the full epoch rewards up to the `latestClaimableEpoch`. For computing the reward of full epochs, the basis includes both the user's original `stakeAmount` and the first epoch's partial reward. Although the reward for the first epoch's partial period isn't actually claimed during its calculation, we aim to verify with the team whether the principal for calculating rewards for full epochs should indeed include the first epoch. Therefore, what we want to confirm with the team is the reason why the first epoch's partial reward is also used as the principal when calculating full epochs’ rewards.

The project design may not be compatible with non-standard ERC20 tokens, such as deflationary tokens or rebase tokens. The functions use `transferFrom()` to move funds from the sender to the recipient but fail to verify if the received token amount matches the transferred amount. This could pose an issue with fee-on-transfer tokens, where the post-transfer balance might be less than anticipated, leading to balance inconsistencies. There might be subsequent checks for a second transfer, but an attacker might exploit leftover funds (such as those accidentally sent by another user) to gain unjustified credit.

There are discrepancies between the pointed comment and code implementation in the runtime configuration: In the `Staking` contract, there are the following inconsistencies between code and comments: - In the `_getDaysUntilEpochEnd()` function, the code first calculates the timestamp and then divides by `1 day`, whereas the comment suggests dividing by `1 day` first. - In the `_getPeriodCompoundInterestRate()` function, the comment indicates that positive rates are derived by subtracting 1 from the calculation result, and negative rates are obtained by subtracting the calculation result from 1. However, the code does not involve any operations related to 1.

This loop iterates from the last epoch a stake was claimed for a given `stakeId` up to the `latestClaimableEpoch`, triggering an `EpochClaimed` event for each. However, if `stakeLastClaimedEpoch` is 0, the epoch of the event should begin at `stakeFirstEpoch` for that specific `stakeId`, rather than starting from 1.

When updating upgradeable smart contracts for new features or bug fixes, it's essential to keep the state variables' declaration order unchanged to avoid storage layout issues. A practical solution is to include unused state variables or explicitly named storage gaps (like `__gap`) in the base contracts. This foresight allows reserved slots for future use, ensuring that any additions to the contract's state won't disrupt the storage pattern of derived contracts or the compatibility with previously deployed versions. The problem of "Lack of Storage Gap in Upgradeable Contract" occurs when these storage gaps are not incorporated into the base contract's logic. As a result, if new state variables are added to the base contract, they might overwrite existing variables in the child contracts due to storage slot collisions. For detailed guidelines and best practices, refer to the following OpenZeppelin documentation: - https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps - https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#storage-gaps

There should always be events emitted in the sensitive functions that are controlled by centralization roles.