Canto contest - robee's results

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

Unnecessary array boundaries check when loading an array element twice

There are places in the code (especially in for-each loops) that loads the same array element more than once.
In such cases, only one array boundaries check should take place, and the rest are unnecessary.
Therefore, this array element should be cached in a local variable and then be loaded
again using this local variable, skipping the redundant second array boundaries check:

Code instances:

    BaseV1-periphery.sol._swap - double load of routes[i]
    BaseV1-periphery.sol.getAmountsOut - double load of routes[i]

Caching array length can save gas

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory. We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length
for (uint256 i=0; i<len; i++) { ... }

Code instances:

    SushiMakerKashi.sol, kashiPair, 97
    BaseV1-periphery.sol, routes, 362
    BaseV1-periphery.sol, routes, 136

Prefix increments are cheaper than postfix increments

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

Code instances:

    just change to unchecked: MasterChefV2.sol, i, 177
    change to prefix increment and unchecked: SushiMaker.sol, i, 113
    just change to unchecked: MiniChefV2.sol, i, 162
    just change to unchecked: MasterChef.sol, pid, 204
    change to prefix increment and unchecked: SushiMakerKashi.sol, i, 97
    change to prefix increment and unchecked: BaseV1-periphery.sol, i, 362
    change to prefix increment and unchecked: BaseV1-periphery.sol, i, 136

Unnecessary index init

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

Code instances:

    MasterChef.sol, 204
    SushiMakerKashi.sol, 97
    BaseV1-periphery.sol, 362
    MiniChefV2.sol, 162
    BaseV1-periphery.sol, 136
    SushiMaker.sol, 113
    MasterChefV2.sol, 177

Storage double reading. Could save SLOAD

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

Code instance:

    SushiMaker.sol: weth is read twice in _convertStep

Unnecessary default assignment

Unnecessary default assignments, you can just declare and it will save gas and have the same meaning.

Code instance:

    MasterChef.sol (L#75) : uint256 public totalAllocPoint = 0;

Short the following require messages

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

Code instances:

    Solidity file: BaseV1-periphery.sol, In line 104, Require message length to shorten: 33, The message: BaseV1Router: INSUFFICIENT_AMOUNT
    Solidity file: SushiToken.sol, In line 120, Require message length to shorten: 35, The message: SUSHI::delegateBySig: invalid nonce
    Solidity file: BaseV1-periphery.sol, In line 296, Require message length to shorten: 35, The message: BaseV1Router: INSUFFICIENT_B_AMOUNT
    Solidity file: BaseV1-periphery.sol, In line 452, Require message length to shorten: 35, The message: TransferHelper: ETH_TRANSFER_FAILED
    Solidity file: BaseV1-periphery.sol, In line 105, Require message length to shorten: 36, The message: BaseV1Router: INSUFFICIENT_LIQUIDITY
    Solidity file: MasterChefV2.sol, In line 95, Require message length to shorten: 35, The message: MasterChefV2: Balance must exceed 0
    Solidity file: BaseV1-periphery.sol, In line 295, Require message length to shorten: 35, The message: BaseV1Router: INSUFFICIENT_A_AMOUNT
    Solidity file: BaseV1-periphery.sol, In line 86, Require message length to shorten: 33, The message: BaseV1Router: IDENTICAL_ADDRESSES

Use != 0 instead of > 0

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

Code instances:

    BaseV1-periphery.sol, 104: change 'amountA > 0' to 'amountA != 0'
    BaseV1-periphery.sol, 105: change 'reserveB > 0' to 'reserveB != 0'
    BaseV1-periphery.sol, 105: change 'reserveA > 0' to 'reserveA != 0'
    SushiToken.sol, 197: change 'amount > 0' to 'amount != 0'
    SushiToken.sol, 226: change 'nCheckpoints > 0' to 'nCheckpoints != 0'

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    SushiToken.sol, safe32
    SushiRoll.sol, _addLiquidity
    SushiRoll.sol, removeLiquidity
    SushiToken.sol, getChainId

Cache powers of 10 used several times

You calculate the power of 10 every time you use it instead of caching it once as a constant variable and using it instead. Fix the following code lines:

Code instances:

BaseV1-core.sol, 115 : decimals0 = 10erc20(_token0).decimals(); BaseV1-core.sol, 116 : decimals1 = 10erc20(_token1).decimals();

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

    1. Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
    2. Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
    3. Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
    4. Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Code instances:

    SushiRoll.sol
    SushiToken.sol
    MasterChefV2.sol
    SushiBar.sol
    Migrator.sol
    MiniChefV2.sol
    SushiMaker.sol
    SushiMakerKashi.sol
    Ownable.sol
    MasterChef.sol

Do not cache msg.sender

We recommend not to cache msg.sender since calling it is 2 gas while reading a variable is more.

Code instances:

    stableswap/contracts/BaseV1-core.sol#L490
    zeroswap/contracts/Ownable.sol#L25

State variables that could be set immutable

In the following files there are state variables that could be set immutable to save gas.

Code instances:

    router in SushiRoll.sol
    oldRouter in SushiRoll.sol
    sushi in SushiBar.sol