Migration functionality possible pausing

High  MH – 01 |  Not relevant

The Migrator contract was not provided to the audit. If an incorrect migrator address will be passed to the Masterchef contract the migrate() function may fail.

In case, when more reward or staked tokens will be withdrawn with safeApprove() function user calls (withdraw(), leaveStaking(), emergencyWithdraw() functions) could be failed if token balance of the contract will be insufficient.

Recommendation:

Make sure that Migrator contract will proceed correctly.

Re-Audit:

Not relevant after removing functionality.

Access to withdraw all tokens from Masterchef SC

High  MH – 02 |  Resolved

The safeApprove() can approve a spender to withdraw all tokens by owner (including EG and LP tokens from the contract).

Recommendation:

Make sure that transferring more tokens after safeApprove() will not break operating of the contract.

Access to withdraw tokens of EGSwapPair by feeToSetter address

High  MH – 03 |  Resolved

In the EGSwapPair Smart Contract in the initialize() function there is two lines:

IERC20(token0).approve(IEGSwapFactory(factory).feeToSetter(), uint256(-1));
IERC20(token1).approve(IEGSwapFactory(factory).feeToSetter(), uint256(-1));

that makes possibility to withdraw pair tokens of users from the contract by feeToSetter. It can decrease user trust to the project.

Recommendation:

Provide the technical description of needing these lines or remove it.

Inappropriate function visibility

Low ML – 01 |  Resolved

In the audited codebase there are some functions that not called in the contract itself so their visibility can be improved from public to external:

• function updateMultiplier();
• function setEmergencyFee();
• function add();
• function set();
• function deposit();
• function withdraw();
• function enterStaking();
• function leaveStaking();
• function emergencyWithdraw();
• function safeApprove();
• function dev();

Recommendation:

Consider changing visibility from public to external to safe gas usage on calling these functions.

Re-Audit:

Resolved but missed for emergencyWithdraw() function.

Re-Audit 2:

Resolved.

Useless IF statement

Low ML – 02 |  Resolved

There is a duplicated if statement in the withdraw() function of the Masterchef contract (L1208).

Recommendation:

Remove duplicated if (pending > 0):

if (pending > 0) {
            if (pending > 0) {
		             ...
            }
        }

msg.sender usage

Low ML – 03 |  Resolved

_msgSender() function can be used instead of msg.sender in Masterchef Smart Contract because it is inherited from Context Smart Contract. The _msgSender() function call requires less gas than msg.sender.

Recommendation:

Consider changing msg.sender to _msgSender().

Re-Audit:

Not resolved for calls in the emergencyWithdraw() function.

Re-Audit 2:

Resolved.

Optimization by ternary operator

Low ML – 04 |  Not relevant

In the Materchef Smart Contract there is deposit() functions that can be optimized to process result of ternary operator to transfer EG that will decrease using of if-else branching:

if (pending > 0) {
                if (validateStakingTime(msg.sender)) {
                    safeEGTransfer(msg.sender, pending, user.referral);
                } else {
                    safeEGTransfer(
                        msg.sender,
                        pending.mul(95).div(100),
                        user.referral
                    );
                }
            }

to:

if (pending > 0)
                safeEGTransfer(
                    msg.sender,
                    validateStakingTime(msg.sender)
                        ? pending
                        : pending.mul(95).div(100),
                    user.referral
                );

Recommendation:

Consider using ternary operators. It can also be used in the withdraw(), enterStaking(), leaveStaking() functions and validateStakingTime():

function validateStakingTime(address _staker) public view returns (bool) {
        UserInfo memory user = userInfo[0][_staker];
        if ((user.stakingTime + 30 days) < block.timestamp) {
            return true;
        }
        return false;
    }

to:

function validateStakingTime(address _staker) public view returns (bool) {
        return (userInfo[0][_staker].stakingTime + 30 days) < block.timestamp;
    }

Re-Audit:

Not relevant after logic changing.

Unnecessary default initialization

Low ML – 05 |  Resolved

There are state variables in Masterchef contract that are initialized by default type value that can be avoided:

• uint256 public totalAllocPoint = 0;
• uint256 public totalStakeAmount = 0; (or could be initialized with 1000 to miss initializing in the constructor);
• uint256 points = 0; (in the updateStakingPool() function);
• uint256 pid = 0; (in the massUpdatePools() function).

Recommendation:

Consider removing initialization by the default type value in the described cases.

Lack of delete operator

Low ML – 06 |  Resolved

In function emergencyWithdraw() assigns zero value to few struct fields. It is more efficient to use delete operator here to save gas usage on the function execution:

• delete user.amount; (line 1303);
• delete user.rewardDebt; (line 1304).

Recommendation:

Consider using delete operator to “clear” variables.

Lack of zero value check

Low ML – 07 |  Resolved

In function setMigrator() the migrator address is not checked if the passed address isn’t zero address. Same for the dev() function – if dev will be zero address then all transferred fees will be burned.

Recommendation:

Consider adding a requirement statement to check if the value isn’t zero value.

Unnecessary pairFor() internal call

Low ML – 08 |  Resolved

There is pairFor() call in the getReserves() function of EGSwapRouter contract (line 303) that returns a CREATE2 address for a pair but it’s not stored to the variable and duplicates the call in the next line.

Recommendation:

Consider removing the pairFor() call in the getReserves() function (line 303).

Simplifying the comparison for the requirement

Low ML – 09 |  Resolved

There is the requirement in the add() function of Masterchef contract (line 1051):

require(poolExist[address(_lpToken)] != true, "Existing pool");

that could be simplified to decrease gas usage:

require(!poolExist[address(_lpToken)], "Existing pool");

Recommendation:

Consider changing requirement for gas optimization.

Using of safe functions from SafeMath and SafeBEP20

Low TL – 01 |  Resolved

There are missed few points of using safe functions in Masterchef contract:

totalStakeAmount += _amount;

to:

totalStakeAmount = totalStakeAmount.add(_amount);

In safeEGTransfer() function:

if (_amount > egBal) {
            eg.transfer(_to, egBal);
        } else {
            eg.transfer(_to, _amount);
        }

to:

if (_amount > egBal) {
            eg.safeTransfer(_to, egBal);
        } else {
            eg.safeTransfer(_to, _amount);
        }

In emergencyWithdraw() function:

totalStakeAmount -= amount;

to:

totalStakeAmount = totalStakeAmount.sub(amount);

In emergencyWithdraw() function:

amount -= amount.mul(emergencyFee).div(100);

to:

amount = amount.sub(amount.mul(emergencyFee).div(100));

In leaveStaking() function:

totalStakeAmount -= _amount;

to:

totalStakeAmount = totalStakeAmount.sub(_amount);

In leaveStaking() function:

_amount -= unlockingFee;

to:

_amount = _amount.sub(unlockingFee);

Recommendation:

Use safe functions for calculating values and transferring tokens in all cases.

Lack of NatSpec comments

Informational  MI – 01 |  Unresolved

EGSwapFactory, EGSwapRouter, Masterchef and dependencies contracts are not covered by NatSpec. There are some functions that are covered only by simple comments. Follow:

• https://docs.soliditylang.org/en/v0.5.16/natspec-format.html;
• https://docs.soliditylang.org/en/v0.6.6/natspec-format.html;
• https://docs.soliditylang.org/en/v0.6.12/natspec-format.html.

Recommendation:

Consider covering all functions with qualitative and fully NatSpec comments.

Re-Audit:

Not resolved, only some comments were added. Check the examples with links added in the issue, use triple-slash and tags (@author, @notice, @param etc) to write NatSpec documentation.

Typo in the codebase

Informational  MI – 02 |  Resolved

There are some typos in the contracts code:

There is typo in comment, appears in Masterchef contract: poitns -> points.

Recommendation:

Consider fixing typos.

Wrong variable naming convention

Informational  MI – 03 |  Resolved

According to Solidity Style Guide, state and local variables should be in mixedCase style. In the Masterchef contract there are such variables that could be changed:

• devaddr -> devAddr;
• BONUS_MULTIPLIER -> bonusMultiplier (only constants can be written with uppercase).

Recommendation:

Consider following the Solidity style guide 0.6.12 and naming conventions.

‍