Froggies security record

The `emergencyUnstake()` function allows users to withdraw their staked tokens anytime. While users who call this function will incur a penalty, the penalty rate is capped at 25%. Therefore, if the reward rate during the period is sufficiently high, malicious users still have the opportunity to exploit the system by repeatedly calling `stake()` and `emergencyUnstake()` to arbitrage the rewards. We want to remind our client and the users of the contract that this issue can be exploited through flash loans, resulting in a significant loss of funds for the contract. Flash loans are a way to borrow large amounts of money for a certain fee. The requirement is that the loans need to be returned within the same transaction in a block. If not, the transaction will be reverted. An attacker can use the borrowed money as the initial funds for an exploit to enlarge the profit.

When users call the `emergencyUnstake()` function, their current rewards are calculated, and a certain percentage of penalties are applied based on the amount staked by the user and the uncollected rewards. The user will receive the remaining portion after the penalty deduction. ```solidity=152 calculateReward(msg.sender); uint256 uncollectedReward = stakers[msg.sender].totalRewardAllocation.sub(stakers[msg.sender].reward); uint256 total = stakers[msg.sender].amount.add(uncollectedReward); uint256 penalty = total.mul(emergencyWithdrawalPenalty).div(100); uint256 burnAmount = penalty.mul(burnRateEmergency).div(100); burnAmount = burnAmount.add(stakers[msg.sender].burnAmount); uint256 poolAmount = penalty.sub(burnAmount); // Transfer the remaining amount after penalty to the user token.transfer(msg.sender, total.sub(penalty)); ``` However, besides the uncollected rewards, a portion of rewards may have already been generated but are not distributed in the `emergencyUnstake()` function. This results in these rewards being permanently locked in the contract, making them inaccessible for the `owner` to withdraw or distribute as rewards to other stakers. We would like the client to teach us whether the inconsistency between the variable `stakingPool` and the contract balance is intended or part of the design. We also recommend implementing proper logic to handle undistributed rewards.

The calculation of rewards does not necessarily end with the conclusion of the `period`, which means users may receive extra rewards beyond that `period`. ```solidity=102 function calculateReward(address _staker) internal { uint256 timeElapsed = block.timestamp.sub(stakers[_staker].lastRewardCalculation); uint256 rewardPerSecond = stakers[_staker].totalRewardAllocation.div(stakers[_staker].stakePeriod); uint256 newReward = rewardPerSecond.mul(timeElapsed); stakers[_staker].reward = stakers[_staker].reward.add(newReward); stakers[_staker].lastRewardCalculation = block.timestamp; totalAllocated = totalAllocated.add(newReward); } ``` In the function `calculateReward()`, the `block.timestamp` is used for calculating the local variable `timeElapsed` without sanity validation. This results in the issue where rewards continue to be generated even after the period has ended.

`if` branches in lines 187 and 199 have the same logic issue. If the execution steps into the `if` branch in line 187, the fee `lpAmount` will not be added to the state variable `lpCurrentAmount`, and this contract can not receive the `lp` fee. Similarly, if the execution steps into the `if` branch in line 199, the fee `marketingAmount` will not be added to the state variable `marketingCurrentAmount`, and this contract can not receive the `marketing` fee. ```solidity=185 if (sellFees.lp > 0) { uint256 lpAmount = remainingAmount.mul(sellFees.lp).div(100); if (lpThreshold != 0 && (lpCurrentAmount.add(lpAmount) >= lpThreshold)) { swapAndLiquify(lpCurrentAmount); lpCurrentAmount = 0; // Reset the lpCurrentAmount to 0 after swapAndLiquify is called remainingAmount = remainingAmount.sub(lpAmount); } else { lpCurrentAmount = lpCurrentAmount.add(lpAmount); remainingAmount = remainingAmount.sub(lpAmount); super._transfer(from, address(this), lpAmount); } } if (sellFees.marketing > 0) { uint256 marketingAmount = remainingAmount.mul(sellFees.marketing).div(100); if (isSwap && marketingThreshold != 0 && (marketingCurrentAmount.add(marketingAmount) >= marketingThreshold)) { _swapTokensForBNBSimple(marketingCurrentAmount); marketingCurrentAmount = 0; // Reset the marketingCurrentAmount to 0 because all tokens have been swapped remainingAmount = remainingAmount.sub(marketingAmount); } else { marketingCurrentAmount = marketingCurrentAmount.add(marketingAmount); remainingAmount = remainingAmount.sub(marketingAmount); super._transfer(from, address(this), marketingAmount); } } ```

The function allows users to set the `_burnRate` parameter, and there doesn't seem to be a lower limit check on this value. As a result, users can potentially set `_burnRate` to 0 to maximize their rewards. Meanwhile, users can freely specify the `_stakePeriod` parameter. We would appreciate it if the client could confirm whether this logic aligns with the intended design or if any additional constraints or considerations are in place.

When transferring deflationary ERC20 tokens, the input amount may not be equal to the received amount due to the charged transaction fee. For example, if a user sends 100 deflationary tokens (with a 10% transaction fee), only 90 tokens actually arrived to the contract. However, a failure to discount such fees may allow the same user to withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction. ```solidity=81 token.transferFrom(msg.sender, address(this), _amount); ```

The state variable `_inSwapAndLiquify` is not used in any validation logic, therefore the modifier `lockTheSwap` can't move as an execution lock.

**Regarding the withdrawal of rewards, we have the following questions. Please confirm whether the logic for this part is intended and aligned with the design or if there might be issues with implementing this code.** If users withdraw rewards during the period, they may not be able to receive the total amount of rewards they are entitled to. Each time the function `withdrawReward()` is called, the claimed reward amount is deducted from the field `Staker.totalRewardAllocation` property. ```solidity=113 function withdrawReward() external nonReentrant { require(stakers[msg.sender].amount > 0, "You are not staking"); calculateReward(msg.sender); uint256 reward = stakers[msg.sender].reward; stakers[msg.sender].reward = 0; stakers[msg.sender].totalRewardAllocation = stakers[msg.sender].totalRewardAllocation.sub(reward); token.transfer(msg.sender, reward); } ``` When a user calls the `withdrawReward()` function again or executes the `unstake()` function, since the value of the field `Staker.totalRewardAllocation` is reduced and the reward can generate per second depends on the `Staker.totalRewardAllocation`, the reward amount they can claim will be reduced. If a user withdraws the rewards in the period multiple times, the reward generation will not be linear. ```solidity=102 function calculateReward(address _staker) internal { uint256 timeElapsed = block.timestamp.sub(stakers[_staker].lastRewardCalculation); uint256 rewardPerSecond = stakers[_staker].totalRewardAllocation.div(stakers[_staker].stakePeriod); ``` If users indeed receive fewer rewards by withdrawing them before the end of the period, the portion of rewards they miss out on should be returned to the staking pool. Because when users stake in the staking pool, a theoretical total reward amount is calculated, which is frozen from the total reward pool for future distribution. If the total rewards that users can receive can decrease, this frozen amount will remain locked indefinitely, which hinders its use for rewarding other users or withdrawing it, affecting the efficiency of fund utilization. ```solidity=97 stakingPool = stakingPool.sub(totalAllocation); ```

The linked state variables are never used in calculation or query.