Cudos contest - robee's results

Does not validate the input fee parameter

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

Code instance:

    Gravity.submitBatch (_fees)

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 instance:

    Deprecated safeApprove in TestUniswapLiquidity.sol line 29: IUniswapV2Pair(pair).approve(router, 2**256 - 1);

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:

    TestUniswapLiquidity.sol.redeemLiquidityETH token
    TestUniswapLiquidity.sol.transferTokens _to
    ReentrantERC20.sol.transfer _recipient
    SimpleLogicBatch.sol.logicBatch _logicContract
    CudosAccessControls.sol.removeAdminRole _address

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 SimpleLogicBatch.sol doesn't use safe math and is of solidity version < 8
    The contract TestTokenBatchMiddleware copy.sol doesn't use safe math and is of solidity version < 8
    The contract TestERC20B.sol doesn't use safe math and is of solidity version < 8
    The contract CudosAccessControls.sol doesn't use safe math and is of solidity version < 8
    The contract TestUniswapLiquidity.sol doesn't use safe math and is of solidity version < 8

Missing non reentrancy modifier

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

Code instances:

    Gravity.sol, withdrawERC20 is missing a reentrancy modifier
    Gravity.sol, constructor is missing a reentrancy modifier
    Gravity.sol, manageWhitelist is missing a reentrancy modifier
    Gravity.sol, deployERC20 is missing a reentrancy modifier

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 instance:

    Gravity.sol, updateValset

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:

    ReentrantERC20.sol: function transfer parameter _amount isn't used. (transfer is public)
    ReentrantERC20.sol: function transfer parameter _recipient isn't used. (transfer is public)

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:

    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/TestUniswapLiquidity.sol#L68
    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/Gravity.sol#L344
    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/SimpleLogicBatch.sol#L36
    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/Gravity.sol#L454
    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/TestTokenBatchMiddleware copy.sol#L17

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 instance:

    Gravity.sol, updateValset

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:

    ReentrantERC20.sol: function transfer parameter _amount isn't used. (transfer is public)
    ReentrantERC20.sol: function transfer parameter _recipient isn't used. (transfer is public)

Must approve 0 first

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

Code instance:

approve without approving 0 first TestUniswapLiquidity.sol, 29, IUniswapV2Pair(pair).approve(router, 2**256 - 1);

Two arrays length mismatch

The functions below fail to perform input validation on arrays to verify the lengths match. A mismatch could lead to an exception or undefined behavior. Consider making this a medium risk please.

Code instances

    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/Gravity.sol#L219 checkValidatorSignatures ['_currentValidators', '_currentPowers', '_v', '_r', '_s']
    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/SimpleLogicBatch.sol#L27 logicBatch ['_amounts', '_payloads']
    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/TestTokenBatchMiddleware copy.sol#L10 submitBatch ['_amounts', '_destinations']
    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/HashingTest.sol#L14 IterativeHash ['_validators', '_powers']

Tokens with fee on transfer are not supported

There are ERC20 tokens that charge fee for every transfer() / transferFrom().

Vault.sol#addValue() assumes that the received amount is the same as the transfer amount, and uses it to calculate attributions, balance amounts, etc. But, the actual transferred amount can be lower for those tokens. Therefore it's recommended to use the balance change before and after the transfer instead of the amount. This way you also support the tokens with transfer fee - that are popular.

Code instance:

    https://github.com/code-423n4/2022-05-cudos/tree/main/solidity/contracts/Gravity.sol#L514

approve return value is ignored

Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value. We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens. The list of occurrences in format (solidity file, line number, actual line)

Code instance:

TestUniswapLiquidity.sol, 29, IUniswapV2Pair(pair).approve(router, 2**256 - 1);

transfer return value of a general ERC20 is ignored

Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must

1. Check the transfer return value Another popular possibility is to add a whiteList. Those are the appearances (solidity file, line number, actual line):

Code instances:

    TestUniswapLiquidity.sol, 68 (transferTokens), IERC20(state_tokenContract).transfer(_to, _a + _b);
    TestLogicContract.sol, 21 (transferTokens), IERC20(state_tokenContract).transfer(_to, _a + _b);

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:

    state_tokenContract in TestLogicContract.sol
    state_gravityAddress in ReentrantERC20.sol
    router in TestUniswapLiquidity.sol

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:

    SimpleLogicBatch.sol, _amounts, 35
    Gravity.sol, transferAmounts._args, 568
    Gravity.sol, _powers, 660
    Gravity.sol, _users, 128
    TestTokenBatchMiddleware copy.sol, _amounts, 16

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:

    change to prefix increment and unchecked: Gravity.sol, i, 579
    change to prefix increment and unchecked: Gravity.sol, i, 568
    change to prefix increment and unchecked: Gravity.sol, i, 660
    change to prefix increment and unchecked: Gravity.sol, i, 233
    change to prefix increment and unchecked: SimpleLogicBatch.sol, i, 35

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:

    Gravity.sol, 263
    Gravity.sol, 660
    Gravity.sol, 579
    TestTokenBatchMiddleware copy.sol, 16
    Gravity.sol, 568

Unnecessary default assignment

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

Code instance:

    Gravity.sol (L#54) : uint256 public state_lastValsetNonce = 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 instance:

    Gravity.sol, verifySig

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:

    ReentrantERC20.sol
    TestUniswapLiquidity.sol
    SimpleLogicBatch.sol
    TestERC20B.sol
    SigningTest.sol

Gas Optimization On The 2^256-1

Some projects (e.g. Uniswap - https://github.com/Uniswap/interface/blob/main/src/hooks/useApproveCallback.ts#L88) set the default value of the user's allowance to 2^256 - 1. Since the value 2^256 - 1 can also be represented in hex as 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff. From Ethereum's yellow paper we know that zeros are cheaper than non-zero values in the hex representation. Considering this fact, an alternative choice could be now 0x8000000000000000000000000000000000000000000000000000000000000000 or 2^255 to represent "infinity". If you do the calculations with Remix, you will see that the former costs 47'872 gas, while the latter costs 45'888 gas. If you accept that infinity can also be represented via 2^255 (instead of 2^256-1), which almost all projects can - you can already save about 4% gas leveraging this optimisation trick on those calculations.

Code instances:

    CosmosToken.sol (L#5): uint256 MAX_UINT = 2**256 - 1; )
    TestUniswapLiquidity.sol (L#30): IUniswapV2Pair(pair).approve(router, 2**256 - 1); )