Sakai Vault security record

The report gives no description here.

According to the logic, the initial value of `voteAddress` is zero address, after the first epoch, any user who stakes `minimumStakeForVote` tokens can vote. When `voteAddress` is not zero address, the contract will get current epoch from vote contract for prevent user voting on running epoch. ```solidity shares[account].lastEpochNumberWhenDeposit = voteAddress != address(0) ? ITokenVote(voteAddress).getCurrentEpoch() : 0; ``` However, once the `voteAddress` is set, it cannot be changed back to the zero address. Whether the user can vote will be affected by the `voteAddress` contract. ```solidity function updateVoteAddress(address _voteAddress) external onlyOwner { require(_voteAddress != address(0), "SakaiDAO: Invalid address"); require(voteAddress != _voteAddress, "SakaiDAO: Vote address is already that address"); voteAddress = _voteAddress; emit UpdateVoteAddress(_voteAddress); } ```

If it is intended to prohibit token transfers, it is better to use `revert` to terminate the transaction and throw an error. This is because: 1. Clarity: `revert` clearly indicates that the transaction has failed and provides a reason for the failure. This is helpful for both users and developers as it provides clear feedback on why the operation is not allowed. 2. Gas Savings: When a transaction is reverted, the unused Gas is refunded to the user. Executing empty logic, although it does not change the state, still incurs Gas costs for these invalid computations. 3. Security: Executing empty logic may mislead users into thinking the transaction was successful. Using `revert` can avoid such misunderstandings and ensure that the contract behaves in accordance with its design intentions, enhancing contract security. Therefore, for the sake of transparency, Gas savings, and ensuring security, it is recommended to use `revert` when prohibiting token transfers.

According to the design, when a user unstakes, the contract estimates the fee based on the duration of the staking. The longer the staking period, the lower the fee, and if the staking period exceeds 365 days, no fee is charged when the user unstakes. However, the contract uses `shares[account].lastDepositTimestamp` as the staking start time, and this state variable will be updated both when the user stakes and unstakes. This means that if a user unstakes (possibly only a portion), the staking duration for the user will be recalculated. ```solidity function _estimateTaxPercentEarlyUnstake(address account) internal view returns(uint256) { /** this function for estimate tax if user unstake early */ uint256 lastDepositTimestamp = shares[account].lastDepositTimestamp; uint256 totalDaysStake = (block.timestamp - lastDepositTimestamp) / 86400; uint256 tax = 0; if(totalDaysStake >=0 && totalDaysStake <= 90) tax = 250; else if(totalDaysStake > 90 && totalDaysStake <= 120) tax = 223; else if(totalDaysStake > 120 && totalDaysStake <= 150) tax = 196; else if(totalDaysStake > 150 && totalDaysStake <= 180) tax = 169; else if(totalDaysStake > 180 && totalDaysStake <= 210) tax = 142; else if(totalDaysStake > 210 && totalDaysStake <= 240) tax = 115; else if(totalDaysStake > 240 && totalDaysStake <= 270) tax = 88; else if(totalDaysStake > 270 && totalDaysStake <= 300) tax = 61; else if(totalDaysStake > 300 && totalDaysStake <= 330) tax = 34; else if(totalDaysStake > 330 && totalDaysStake <= 365) tax = 7; return tax; } ```

The owner of the **SakaiDAO** contract can claim all tokens in the contract except the **Sakai** token, but the error message indicates that the owner can only claim native tokens, which is inconsistent with the requirements.

The linked code does not affect the functionality of the codebase and appears to be either remnants of test code or older functionality.

It is not recommended to use Solidity's `transfer()` and `send()` functions for transferring native tokens, since some contracts may not be able to receive the funds. Those functions forward only a fixed amount of gas (2300 specifically) and the receiving contracts may run out of gas before finishing the transfer. Also, EVM instructions' gas costs may increase in the future. Thus, some contracts that can receive now may stop working in the future due to the gas limitation. ```solidity=797 payable(msg.sender).transfer(address(this).balance); ``` - `SakaiDAO.claimStuckBNB` uses `transfer()`.

`SAKAI` token charges 3% buy/sell fee, and the 5/6 of the fee is collected by `treasuryWallet` and `gasWallet`. If these fees don't flow back to the users in some way, eventually all the SAKAI token would end up in the contract owner's wallet. We'd like to confirm if there's a way for the users/token holders to benefit in the tokenomics by design.

The `stakingToken` should not be deflationary token. 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 Specifically in `Sakaivault`, in function `deposit()`, the `safeTransferFrom()` amount is the same as the recorded stake amount. But for deflationary tokens, the actual received amount might be less. ```solidity=1617 ERC20token.safeTransferFrom(msg.sender, address(this), amount); ``` ```solidity=1643 _stakers[msg.sender].balance += amount; ``` On the other hand, function `withdraw()` still transfers out the recorded staked balance. ```solidity=1657 IERC20 ERC20token = IERC20(stakingToken); ERC20token.safeTransfer(msg.sender, balance); totalAddressStaked -= 1; totalStakedToken -= balance; ```

A sandwich attack may happen when an attacker observes a transaction swapping tokens or adding liquidity without setting restrictions on slippage or minimum output amount. The attacker can manipulate the exchange rate by frontrunning (executing before the target) a transaction to purchase one of the assets and make profits by backrunning (executing after the target) a transaction to sell the asset. The following functions are called without setting restrictions on slippage or minimum output amount, so transactions triggering these functions are vulnerable to sandwich attacks, especially when the input amount is large: - `swapExactTokensForTokensSupportingFeeOnTransferTokens()` - `swapExactTokensForETHSupportingFeeOnTransferTokens()` This poses as an issue since the `manualSwapBack()` function is callable by anyone. It might be possible for an attacker to manipulate the price of the SAKAI-USDT and/or SAKAI-WETH pool using flashloan, and then call `manualSwapBack()`. This would result in less USDT for `treasuryWallet` and less WETH for `gasWallet`.