Sabai Protocol security record

In the `getRestOfDeposits()` function, the owner can transfer out `_token.balanceOf(address(this)) - totalActualDepositsAmount` tokens after 30 days of the last deposit's `endTS` date. However, if some users do not call the `getReward()` in time after the `planExpired` date, the owner can run `getRestOfDeposits()` to withdraw all contract funds in excess of user deposits. ```solidity=167 require(_token.transfer(msg.sender, _token.balanceOf(address(this)) - totalActualDepositsAmount)); ``` Since the amount `totalActualDepositsAmount` does not incorporate rewards, the execution of `getRestOfDeposits()` can potentially remove all remaining user rewards. Therefore there might be group of users who will not be able to withdraw both their deposits and rewards.

If `isAntisnipe` is set to true, antisnipe contract can transfer tokens in other addresses without approval, which is a extremely dangerous design.

In the `createDeposit()` function, when creating a deposit, the contract saves the generated `deposit_id` into the `depositsCounter` `totalDepositsCount+1` position. In the case of the very first deposit, this would mean that the **0** index is never used. ```solidity depositsCounter[totalDepositsCount+1] = deposit_id; ``` When iterating `depositsCounter` to get the deposit info in the `getAmountToCloseactualDiposits()` and `getRestOfDeposits()` functions, the starting index is **0** and the last index of `depositsCounter` will never be reached. ```solidity for(uint8 i = 0 ; i<totalDepositsCount; i++) ```

The current implementation deducts the amount `deposits[_depositId].claimed` from amount `totalActualDepositsAmount`. This could result in users not being able to retrieve their deposit and rewards. The variable `totalActualDepositsAmount` is updated by adding on the deposit amount each time a new deposit is created. ```solidity function createDeposit(uint256 count) public payable { // ... Omitted totalDepositsAmount += count; totalDepositsCount ++; totalActualDepositsAmount += count; ``` When deposits and rewards are withdrawn through function `getReward()`, the amount of `deposits[_depositId].claimed`, which is `deposits[_depositId].amount` + rewards, is deducted from the amount `totalActualDepositsAmount` which only keeps track of total deposit amounts. ```solidity function getReward(bytes32 _depositId) public onlyOwner { // ... Omitted // If the deposit is completed if (getCurrentTime() > deposits[_depositId].endTS) { deposits[_depositId].claimed = deposits[_depositId].amount + ((deposits[_depositId].amount / 100) * percentageOfEarnings); } else { // If the deposit is closed ahead of schedule (penalty) deposits[_depositId].claimed = deposits[_depositId].amount - ((deposits[_depositId].amount / 100) * penaltyPercentage); } require(_token.transfer(msg.sender, deposits[_depositId].claimed)); totalActualDepositsAmount -= deposits[_depositId].claimed; ``` The issue with this treatment is that when multiple users withdraw deposits and rewards, there is a chance that the last user(s) to claim rewards will be unable to retrieve the full amount since the value of `totalActualDepositsAmount` since reward withdrawals are not accounted for in the amount `totalActualDepositsAmount`.

There is a `onlyOwner` modifier in the public function `getReward()`, which restricts only the owner to call it. However, from its implementation, it checks if the msg.sender matches the `_depositId`, which means it should allow common users to call it.

The `getAmountToCloseactualDiposits()` function is used to calculate how many tokens are required to provide rewards for all active deposits. However, the calculation makes use of the `penaltyPercentage` instead, and it also does not consider completed cases, where `getCurrentTime() > deposits[_depositId].endTS`. The business logic of this function is inconsistent with the `getReward()` function. ```solidity=64 // If the deposit is completed if (getCurrentTime() > deposits[_depositId].endTS) { deposits[_depositId].claimed = deposits[_depositId].amount + ((deposits[_depositId].amount / 100) * percentageOfEarnings); } else { // If the deposit is closed ahead of schedule (penalty) deposits[_depositId].claimed = deposits[_depositId].amount - ((deposits[_depositId].amount / 100) * penaltyPercentage); } ``` The audit team would like to check if this implementation matches the intended design.

The audit team would like to understand how the contract maintains sufficient funds to transfer deposit rewards to beneficiaries and to ensure that transfers of rewards do not fail under normal conditions.

The current implementation deals a penalty deducting the amount directly from the user's deposit amount. In most cases, the penalties usually impact the amount of rewards the user receives rather than reducing the user's deposit amount. The audit team would like to confirm whether this matches the intended design.

Performing integer division before multiplication truncates the low bits, losing the precision of calculation. ### SabaiStaking.sol ```solidity=66 deposits[_depositId].claimed = deposits[_depositId].amount + ((deposits[_depositId].amount / 100) * percentageOfEarnings); ``` --- ```solidity=68 deposits[_depositId].claimed = deposits[_depositId].amount - ((deposits[_depositId].amount / 100) * penaltyPercentage); ``` --- ```solidity=147 totalAmount+= ((deposits[depositsCounter[i]].amount / 100) * penaltyPercentage); ``` --- ```solidity=397 uint256 vestedSlicePeriods = timeFromStart.div(secondsPerSlice); ``` ### Sabai Vesting.sol ```solidity=398 uint256 vestedSeconds = vestedSlicePeriods.mul(secondsPerSlice); ```

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. Reference: https://thoreum-finance.medium.com/what-exploit-happened-today-for-gocerberus-and-garuda-also-for-lokum-ybear-piggy-caramelswap-3943ee23a39f ### SabaiStaking ```solidity=97 require(_token.transferFrom(_msgSender(), address(this), count)); ``` - Transferring tokens by `count`. ```solidity=103 totalActualDepositsAmount += count; ``` - The `count` appears to be used for bookkeeping purposes without compensating the potential transfer fees. ### SabaiVesting ```solidity=302 _token.safeTransfer(beneficiaryPayable, amount); ``` - Transferring tokens by `amount`. ```solidity=301 vestingSchedulesTotalAmount = vestingSchedulesTotalAmount.sub(amount); ``` - This function call executes the following operation. - In `SafeMath.sub`, - `return a - b;` - The `amount` appears to be used for bookkeeping purposes without compensating the potential transfer fees.