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

Moonery.sol:109

The message "Account is already excluded" is changed to "Account is not excluded" .

SSL-01 Incorrect error message

Logical Issue

Moonery.sol:118

The message "Account is already excluded" is changed to "Account is not excluded" .

SSL-02 Redundant code

Logical Issue

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

Moonery.sol:601

owner() changed to address(this)

SSL-05 Variable could be declared as constant

Gas Optimization

Moonery.sol:35

changed to: uint256 private constant _tTotal = 1000000000 10 ** 6 10 ** 9;

SSL-05 Variable could be declared as constant

Gas Optimization

Moonery.sol:39

changed to: string private constant _name = "Moonery";

SSL-05 Variable could be declared as constant

Gas Optimization

Moonery.sol:40

change to: string private constant _symbol = "MNRY";

SSL-05 Variable could be declared as constant

Gas Optimization

Moonery.sol:41

change to: uint8 private immutable _decimals = 9;

SSL-06 Return value not handled

Volatile Code

MooneryUtils.sol:105

Handle return value from Pancakeswap. public returns (uint amountToken, uint amountETH, uint liquidity)

SSL-07 rd party dependencies

Contr ol Flow

-

Changes on 3rd party library means changes on $MNRY contract, we are aware of this.

SSL-08 Missing event emitting

Coding Style

SSL-09 Function and variable naming doesn't match the operating environment

Coding Style

MooneryUtils.sol:54

Function changed to swapTokensForBnb

SSL-09 Function and variable naming doesn't match the operating environment

Coding Style

MooneryUtils.sol:75

Function change to swapBNBForTokens

SSL-09 Function and variable naming doesn't match the operating environment

Coding Style

several locations

ethAmount changed to bnbAmount

SSL-09 Function and variable naming doesn't match the operating environment

Coding Style

several locations

etheReceived change to bnbReceived

SSL-10 Privileged ownership

Centralization / Privilege

see chapter SSL-10

SSL-11 Typos in the contract

Coding Style

Moonery.sol:57

changed to tokensIntoLiquidity

SSL-11 Typos in the contract

Coding Style

SSL-12 The purpose of function deliver

Control Flow

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

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

PreviousCertiK $SMRATNextBounties

Last updated