nfa - staking/buyback/burn

The `depositFutureRewards` function in the `AssetRewardContract` contains a flaw in its validation logic for the `availableFromTime` parameter. The contract incorrectly rejects deposits for future rewards intended for a time later than the current `block.timestamp`, due to a reversed conditional check. In `depositFutureRewards()` function, the below check is incorrectly added to make sure new future reward deposit's `availableFromTime` must be less than or equal to the `block.timestamp` at the time of the deposit. ```solidity=139 if(block.timestamp < availableFromTime) { revert InvalidFutureTime("Cannot set future rewards for a past time"); } ``` - By providing an `availableFromTime` greater than `block.timestamp`, the transaction is incorrectly reverted. This check implementation is against the logic in the function `distributiedReward()` shown below: ```solidity=223 if ((futureReward.availableFromTime <= block.timestamp && futureReward.availableFromTime != 0)) { totalRewards += futureReward.amount; delete futureRewards[i]; // Remove the future reward as it's now part of the total rewards } ``` The impact of this issue is critical because all of the deposited future reward will be used in distribution every time `distributeReward` is called, due to the failure of the "futureReward.availableFromTime <= block.timestamp" check in the function.

The `InfoAggregator` contract allows any external caller to update the `nFAiUserStakesContract` state variable via the `changeNFAiUserStakesContract` function. This function lacks access control mechanisms, making it possible for any user to change the address of the `nFAiUserStakesContract`. As a result, an unauthorized user could redirect functionalities dependent on this address, potentially leading to loss of funds or manipulation of the aggregated bucket information.

The current implementation of the `normalizeAmount` function only scales up the `stakeAmount` when the stake token's decimals are less than those of the reward token. There is no provision for scaling down when the stake token's decimals are equal to or greater. This finding seeks to discuss and confirm the intended behavior when `stakeTokenDecimals >= rewardTokenDecimals` to ensure the reward distributions are fair and aligned with the system's economic design. A stake token has the same or a greater number of decimals than the reward token. Stakeholders and developers need to confirm whether the intention is for the staked amounts to be adjusted downward or remain as is, which could potentially lead to higher rewards than intended.

The project design may not be compatible with non-standard ERC20 tokens, such as deflationary tokens or rebase tokens.

The constructor of the `NFAiRewardManager` contract does not validate whether the `_nuniAddress` parameter is the zero address. This omission could lead to scenarios where the `NUNI` token is inadvertently set to the zero address, causing the contract to fail to interact correctly with the `NUNI` token functions and potentially leading to loss of functionality or other unintended behaviors in the contract's operations.

In the `NFAiUserStakes` contract and `NFAiRewardManager` contract, we can observed that below 2 pairs of functions are very close in terms of logic: - NFAiUserStakes::removeRewardContractFromBucket vs. NFAiRewardManager::removeRewardContractFromBucket - NFAiUserStakes::addRewardContractToBucket vs. NFAiRewardManager::addRewardContractToBucket The only difference is actually additional security checks in functions of `NFAiUserStakes`, and `NFAiRewardManager` will update the `NFAiUserStakes` contract status with similar logic. ```solidity=83 function addRewardContractToBucket( uint256 bucketId, address rewardContractAddress ) external onlyRole(REWARD_MANAGER) { AssetRewardContract rewardContract = AssetRewardContract(rewardContractAddress); rewardBuckets[bucketId].rewardContracts.push(rewardContract); USER_STAKES_CONTRACT.addRewardContractToBucket(bucketId, rewardContractAddress); } ``` ```solidity=99 function removeRewardContractFromBucket( uint256 bucketId, address rewardContractAddress ) external onlyRole(REWARD_MANAGER) { RewardBucket storage bucket = rewardBuckets[bucketId]; uint256 rewardContractsLength = bucket.rewardContracts.length; for (uint256 i; i < rewardContractsLength; ++i) { if (address(bucket.rewardContracts[i]) == rewardContractAddress) { if (i != rewardContractsLength - 1) { bucket.rewardContracts[i] = bucket.rewardContracts[rewardContractsLength - 1]; } bucket.rewardContracts.pop(); break; } } USER_STAKES_CONTRACT.removeRewardContractFromBucket(bucketId, rewardContractAddress); } ``` ```solidity=235 function removeRewardContractFromBucket( uint256 bucketId, address rewardContractsAddress ) external onlyRole(ADMIN_ROLE) { if (buckets[bucketId].id == 0) { revert BucketDoesNotExist("Bucket does not exist"); } uint256 rewardContractsLength = buckets[bucketId].rewardContracts.length; for (uint256 i; i < rewardContractsLength; ++i) { if (address(buckets[bucketId].rewardContracts[i]) == rewardContractsAddress) { if (i != rewardContractsLength - 1) { buckets[bucketId].rewardContracts[i] = buckets[bucketId].rewardContracts[rewardContractsLength - 1]; } buckets[bucketId].rewardContracts.pop(); emit RewardContractRemovedFromBucket(bucketId, rewardContractsAddress); return; } } revert RewardContractNotAssociated("Reward contract not associated with bucket"); } ``` ```solidity=263 function addRewardContractToBucket( uint256 bucketId, address rewardContractsAddress ) external onlyRole(ADMIN_ROLE) { if (buckets[bucketId].id == 0) { revert BucketDoesNotExist("Bucket does not exist"); } if (rewardContractsAddress == address(0)) { revert TokenAddressZero("Token address is zero"); } AssetRewardContract rewardContract = AssetRewardContract(rewardContractsAddress); buckets[bucketId].rewardContracts.push(rewardContract); emit RewardContractAddedToBucket(bucketId, rewardContractsAddress); } ```

The `wNuni` contract includes a validation check `if(amount < 0) revert NegativeAmount();` in both the `mint` and `burn` functions. Given that the `amount` parameter is of type `uint256`, this check is unnecessary because unsigned integers cannot represent negative values in Solidity. This redundant check increases the gas cost for transactions unnecessarily and adds needless complexity to the contract code.