CertiK $SAFEMOON

CertiK audit report on $Safemoon
Due to the CertiK audit of $Safemoon we have to change our $MNRY token contract.

Findings

ID
Category
Severity
Location
Changes
SSL-01 Incorrect error message
Logical Issue
3
Minor
Moonery.sol:109
The message "Account is already excluded" is changed to "Account is not excluded" .
SSL-01 Incorrect error message
Logical Issue
3
Minor
Moonery.sol:118
The message "Account is already excluded" is changed to "Account is not excluded" .
SSL-02 Redundant code
Logical Issue
🔵
Informational
Moonery.sol: 452
Code has been removed
SSL-03
Logical Issue
-
-
BNB in $MNRY contract automatically distribute to our holders by claimBNBReward function
SSL-04 Centralized risk in addLiquidity
Centralization / Privilege
🔴
Major
Moonery.sol:601
owner() changed to address(this)
SSL-05 Variable could be declared as constant
Gas Optimization
🔵
Informational
Moonery.sol:35
changed to: uint256 private constant _tTotal = 1000000000 10 ** 6 10 ** 9;
SSL-05 Variable could be declared as constant
Gas Optimization
🔵
Informational
Moonery.sol:39
changed to: string private constant _name = "Moonery";
SSL-05 Variable could be declared as constant
Gas Optimization
🔵
Informational
Moonery.sol:40
change to: string private constant _symbol = "MNRY";
SSL-05 Variable could be declared as constant
Gas Optimization
🔵
Informational
Moonery.sol:41
change to: uint8 private immutable _decimals = 9;
SSL-06 Return value not handled
Volatile Code
🔵
Informational
MooneryUtils.sol:105
Handle return value from Pancakeswap. public returns (uint amountToken, uint amountETH, uint liquidity)
SSL-07 rd party dependencies
Contr ol Flow
3
Minor
-
Changes on 3rd party library means changes on $MNRY contract, we are aware of this.
SSL-08 Missing event emitting
Coding Style
🔵
Informational
SSL-09 Function and variable naming doesn't match the operating environment
Coding Style
🔵
Informational
MooneryUtils.sol:54
Function changed to swapTokensForBnb
SSL-09 Function and variable naming doesn't match the operating environment
Coding Style
🔵
Informational
MooneryUtils.sol:75
Function change to swapBNBForTokens
SSL-09 Function and variable naming doesn't match the operating environment
Coding Style
🔵
Informational
several locations
ethAmount changed to bnbAmount
SSL-09 Function and variable naming doesn't match the operating environment
Coding Style
🔵
Informational
several locations
etheReceived change to bnbReceived
SSL-10 Privileged ownership
Centralization / Privilege
3
Minor
see chapter SSL-10
SSL-11 Typos in the contract
Coding Style
🔵
Informational
Moonery.sol:57
changed to tokensIntoLiquidity
SSL-11 Typos in the contract
Coding Style
🔵
Informational
SSL-12 The purpose of function deliver
Control Flow
🔵
Informational
Moonery.sol:227
require(!_isExcluded[sender], "Excluded addresses cannot call this function");
SSL- | Possible to gain ownership after renouncing the contract ownership
Logical Issue, Centralization / Privilege
3
Minor
Ownable
see chapter SSL---

SSL-10 Privileged ownership

Description

The owner of contract Safemoon and $MNRY has the permission to:
  1. 1.
    change the address that can receive LP tokens
  2. 2.
    lock the contract
  3. 3.
    exclude/include addresses from rewards/fees
  4. 4.
    set taxFee, liquidityFee and _maxTxAmount
  5. 5.
    enable swapAndLiquifyEnabled
without obtaining the consensus of the community

Recommendation

Renounce ownership when it is the right timing, or gradually migrate to a timelock plus multisig governing procedure and let the community monitor in respect of transparency considerations.

Alleviation

Use OpenZeppelin Ownable, removing _previousowner. We also transparant about above functions activities, see also Token & Liquidity Locks

SSL- | Possible to gain ownership after renouncing the contract ownership

Description

An owner is possible to gain ownership of the contract even if he calls function renounceOwnership torenounce the ownership. This can be achieved by performing the following operations:.
1. Call lock to lock the contract. The variable _previousOwner is set to the current owner.
2. Call unlock to unlock the contract.
3. Call renounceOwnership to leave the contract without an owner.
4. Call unlock to regain ownership.

Recommendation

We advise updating/removing lock and unlock functions in the contract; or removing the renounce Ownership if such a privilege retains at the protocol level. If timelock functionality could be introduced, we recommend using the implementation of Compound finance as reference. Reference:https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol
Removed implementation of Ownable.sol from $Safemoon and use import "@openzeppelin/contracts/access/Ownable.sol" instead. We can't introduce Timelock.sol due the max size of the contract.

Stuck ERC20 tokens inside $MNRY contracts

Description

If a user will make a mistake and choose a wrong function then the token will get stuck inside our contract (contract will not recognize a transaction). There will be no way to extract stuck tokens.

Solution

We have add fallbackRedeem new IERC20 token, with the ability to recover ERC20 tokens stuck in the contract. The function doesn't have the ability to recover $MNRY and Liquidity tokens $CAKE LP. We only send stick ERC20 token to our lottery contract and add this token as rewards for the community. newToken.safeTransfer(address(_lottery), tokenAmount);
/**
* @dev Fallback Redeem tokens. and send to lottery contract
* Purpose, sending other ERC20 tokens to Lottery contract other than
* $MNRY, $CAKE LP
*
*
* @param newToken_ Address of the token
* @param tokenAmount_ Number of tokens to be emitted
*/
function fallbackRedeem(IERC20 newToken_, uint256 tokenAmount_) external nonReentrant {
require(!_isExcluded[_msgSender()], "Excluded addresses cannot call this function");
require(address(newToken_) != address(this), "cannot recover to $MNRY");
require(address(newToken_) != address(pancakePair), "cannot recover to $CAKE LP");
newToken_.safeTransfer(address(_lottery), tokenAmount_);
}

Max contract Size

Description

Due the above changes the Moonery contract exceed the size limit for testnet and mainnet deployment.

Solutions

Shorten error message
Error
Meaning
FB1
fallbackRedeem: cannot recover to $MNRY
FB2
fallbackRedeem: cannot recover to $CAKE LP
AC1
Moonery: Account is not excluded
AC2
Excluded addresses cannot call this function
BEP1
BEP20: transfer amount exceeds allowance
BEP2
BEP20: decreased allowance below zero
BEP3
BEP20: approve from the zero address
BEP4
BEP20: approve to the zero address
BEP5
BEP20: transfer from the zero address
BEP6
BEP20: transfer to the zero address
BEP7
BEP20: transfer to the zero address
RFT1
Moonery: reflectionFromToken Amount must be less than supply
TFR1
Moonery: tokenFromReflection Amount must be less than total reflections