CertiK $SAFEMOON

CertiK audit report on $Safemoon

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

Certik Audit: https://www.certik.org/projects/safemoon

$MNRT contract: https://github.com/moonery-io/moonery-contracts/blob/main/contracts/Moonery.sol

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. change the address that can receive LP tokens

2. lock the contract

3. exclude/include addresses from rewards/fees

4. set taxFee, liquidityFee and _maxTxAmount

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

Alleviation

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

see also: https://docs.google.com/document/d/1Feh5sP6oQL1-1NHi-X1dbgT3ch2WdhbXRevDN681Jv4/

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