QA Report

[code423n4]

Duplicates in array

    You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.

Code instances:

Both MasterChefV2.add pushed the parameter _lpToken without checking if it is already exists in the array lpToken. And similarly, pushed _rewarder to rewarder without checking if exists.

Loss Of Precision

This issue is about arithmetic computation that could have been done more percise. The following are places in the codebase in which you multiplied after the divisions. Doing the multiplications at start lead to more accurate calculations. This is a list of places in the code that this appears (Solidity file, line number, actual line):

Code instances:

    BaseV1-core.sol, 328, return x0*(y*y/1e18*y/1e18)/1e18+(x0*x0/1e18*x0/1e18)*y/1e18;

safeApprove of openZeppelin is deprecated

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) You should change it to increase/decrease Allowance as OpenZeppilin says.

Code instances:

    Deprecated safeApprove in MasterChef.sol line 152: lpToken.safeApprove(address(migrator), bal);
    Deprecated safeApprove in MiniChefV2.sol line 134: _lpToken.approve(address(migrator), bal);
    Deprecated safeApprove in MasterChefV2.sol line 96: dummyToken.approve(address(MASTER_CHEF), balance);
    Deprecated safeApprove in MasterChefV2.sol line 149: _lpToken.approve(address(migrator), bal);

Require with empty message

The following requires are with empty messages. This is very important to add a message for any require. So the user has enough information to know the reason of failure.

Code instances:

    Solidity file: BaseV1-core.sol, In line 498 with Empty Require message.
    Solidity file: BaseV1-core.sol, In line 508 with Empty Require message.
    Solidity file: BaseV1-periphery.sol, In line 456 with Empty Require message.
    Solidity file: BaseV1-periphery.sol, In line 291 with Empty Require message.
    Solidity file: BaseV1-periphery.sol, In line 459 with Empty Require message.
    Solidity file: BaseV1-core.sol, In line 503 with Empty Require message.
    Solidity file: BaseV1-periphery.sol, In line 211 with Empty Require message.
    Solidity file: BaseV1-periphery.sol, In line 210 with Empty Require message.

Require with not comprehensive message

The following requires has a non comprehensive messages. This is very important to add a comprehensive message for any require. Such that the user has enough information to know the reason of failure:

Code instances:

    Solidity file: MasterChef.sol, In line 294 with Require message: dev: wut?

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    SushiMaker.sol.constructor _factory
    BaseV1-periphery.sol.removeLiquidityCANTOWithPermit token
    BaseV1-core.sol.createPair tokenB
    BaseV1-periphery.sol.swapExactTokensForTokens to
    BaseV1-periphery.sol.removeLiquidity to
    SushiRoll.sol.migrateWithPermit tokenB

Solidity compiler versions mismatch

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Code instance:

Use safe math for solidity version <8

You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity.

Code instances:

    The contract SushiRoll.sol doesn't use safe math and is of solidity version < 8
    The contract SushiToken.sol doesn't use safe math and is of solidity version < 8
    The contract Ownable.sol doesn't use safe math and is of solidity version < 8
    The contract Migrator.sol doesn't use safe math and is of solidity version < 8

Not verified owner

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.

Code instance:

    Ownable.sol.transferOwnership newOwner

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instances:

    BaseV1-periphery.sol, quoteRemoveLiquidity
    BaseV1-periphery.sol, getAmountOut

Assert instead require to validate user inputs

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.

Code instances:

    BaseV1-periphery.sol : reachable assert in line 81
    SushiRoll.sol : reachable assert in line 134
    BaseV1-periphery.sol : reachable assert in line 226
    BaseV1-periphery.sol : reachable assert in line 418
    BaseV1-periphery.sol : reachable assert in line 272

In the following public update functions no value is returned

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

Code instances:

    MasterChefV2.sol, massUpdatePools
    MiniChefV2.sol, massUpdatePools
    MasterChef.sol, updatePool
    MasterChef.sol, massUpdatePools

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    BaseV1-periphery.sol: function _safeTransferCANTO parameter to isn't used. (_safeTransferCANTO is internal)
    BaseV1-periphery.sol: function _safeTransferCANTO parameter value isn't used. (_safeTransferCANTO is internal)
    SushiRoll.sol: function removeLiquidity parameter deadline isn't used. (removeLiquidity is internal)

Open TODOs

Open TODOs can hint at programming or architectural errors that still need to be fixed. These files has open TODOs:

Code instances:

Open TODO in SushiMaker.sol line 243 : // TODO: Add maximum slippage? Open TODO in SushiMaker.sol line 250 : // TODO: Add maximum slippage? Open TODO in SushiMaker.sol line 110 : // TODO: This can be optimized a fair bit, but this is safer and simpler for now

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    zeroswap/contracts/MasterChefV2.sol#L96
    zeroswap/contracts/MiniChefV2.sol#L298
    stableswap/contracts/BaseV1-periphery.sol#L291
    stableswap/contracts/BaseV1-periphery.sol#L247
    stableswap/contracts/BaseV1-periphery.sol#L431
    stableswap/contracts/BaseV1-periphery.sol#L388
    zeroswap/contracts/MasterChefV2.sol#L266
    zeroswap/contracts/SushiBar.sol#L49
    zeroswap/contracts/SushiMakerKashi.sol#L124
    stableswap/contracts/BaseV1-periphery.sol#L318

Assert instead require to validate user inputs

    From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix.
    With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam.

Code instances:

    BaseV1-periphery.sol : reachable assert in line 81
    SushiRoll.sol : reachable assert in line 134
    BaseV1-periphery.sol : reachable assert in line 226
    BaseV1-periphery.sol : reachable assert in line 418
    BaseV1-periphery.sol : reachable assert in line 272

In the following public update functions no value is returned

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

Code instances:

    MasterChefV2.sol, massUpdatePools
    MiniChefV2.sol, massUpdatePools
    MasterChef.sol, updatePool
    MasterChef.sol, massUpdatePools

Never used parameters

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

Code instances:

    BaseV1-periphery.sol: function _safeTransferCANTO parameter to isn't used. (_safeTransferCANTO is internal)
    BaseV1-periphery.sol: function _safeTransferCANTO parameter value isn't used. (_safeTransferCANTO is internal)
    SushiRoll.sol: function removeLiquidity parameter deadline isn't used. (removeLiquidity is internal)

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    zeroswap/contracts/MasterChef.sol#L143
    zeroswap/contracts/MasterChefV2.sol#L140
    zeroswap/contracts/MiniChefV2.sol#L118
    zeroswap/contracts/MiniChefV2.sol#L125
    stableswap/contracts/BaseV1-core.sol#L497
    stableswap/contracts/BaseV1-core.sol#L507
    zeroswap/contracts/SushiMakerKashi.sol#L75
    zeroswap/contracts/MiniChefV2.sol#L109
    zeroswap/contracts/SushiMaker.sol#L73
    zeroswap/contracts/MasterChef.sol#L128
    zeroswap/contracts/MasterChefV2.sol#L131

[GalloDaSballo]

Duplicates in array

Valid L

Loss Of Precision

L

safeApprove of openZeppelin is deprecated

Disputed as the instances provided are 0 to non-zero meaning they are fine

 Require with empty message

NC

Require with not comprehensive message

Disputed as it's a developer function

Not verified input

L

 Solidity compiler versions mismatch

NC

Use safe math for solidity version <8

Disagree in lack of evidence of overflow and underflows, lack of safeMath can be a gas optimization (and Sushi is extremely long standing)

Not verified owner

I'm unable to verify the statement, the OZ code I checked has the 0 check

[GalloDaSballo]

Named return issue

R

Assert instead require to validate user inputs

L

In the following public update functions no value is returned

Disputed. The Badger code had an explicit return value which would always return 0 (missing a return), these are functions without a return value

Never used parameters

They are in the curly brackets

Open TODOs

NC

Disagree with the rest.

[GalloDaSballo]

4L 1R 3NC