SKALE contest - robee's results

Title: Duplicates in array Severity: Low Risk

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

    TokenManagerLinker.registerTokenManager pushed (newTokenManager) 

Title: Div by 0 Severity: Medium Risk

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

    SkaleVerifier.sol (L108) hash might be 0)
    MessageProxyForMainnet.sol (L232) messages might be 0)
    SkaleVerifier.sol (L108) hashA might be 0)
    SkaleVerifier.sol (L108) hashB might be 0)
    CommunityPool.sol (L111) gas might be 0)

Title: Never used parameters Severity: Low Risk

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.

    TokenManagerERC1155.sol: function _getTokenInfo parameter erc1155 isn't used. (_getTokenInfo is private)
    DepositBoxERC1155.sol: function _getTokenInfo parameter erc1155 isn't used. (_getTokenInfo is private)

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:
        
        Instead a < b / c use a * c < b. 
    
In all of the big and trusted contracts this rule is maintained.

    SkaleVerifier.sol, 108, if (hashB < Fp2Operations.P / 2 || mulmod(hashB, hashB, Fp2Operations.P) != ySquared || xCoordinate != hashA) {

Title: transfer return value of a general ERC20 is ignored Severity: Low / Med Risk

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

   DepositBoxERC20.sol, 206 (getFunds), ERC20Upgradeable(erc20OnMainnet).transfer(receiver, amount),
   DepositBoxERC721.sol, 192 (getFunds), IERC721Upgradeable(erc721OnMainnet).transferFrom(address(this), receiver, tokenId);
   TokenManagerERC20.sol, 303 (_exit), contractOnSchain.transferFrom(msg.sender, address(this), amount),
   DepositBoxERC20.sol, 120 (depositERC20), ERC20Upgradeable(erc20OnMainnet).transferFrom(
   TokenManagerERC20.sol, 298 (_exit), contractOnSchain.transferFrom(msg.sender, address(this), amount),
   DepositBoxERC721.sol, 116 (depositERC721), IERC721Upgradeable(erc721OnMainnet).transferFrom(msg.sender, address(this), tokenId);
   

Title: Not verified input Severity: Low Risk

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.

    
    DepositBoxERC1155.sol._receiveERC1155 to
    TokenManagerERC20.sol.postMessage sender
    TokenManagerERC1155.sol._exitBatch to
    TokenManagerERC721.sol._exit to

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    Linker.sol, initialize, 183
    Twin.sol, initialize, 95
    CommunityLocker.sol, initialize, 236
    ERC1155ReceiverUpgradeableWithoutGap.sol, __ERC1155Receiver_init, 28
    DepositBoxERC721.sol, initialize, 268
    DepositBoxEth.sol, initialize, 216
    SkaleManagerClient.sol, initialize, 54
    CommunityPool.sol, initialize, 59
    DepositBoxERC1155.sol, initialize, 406
    DepositBox.sol, initialize, 99

Title: Unbounded loop on array can lead to DoS Severity: Low Risk (Kind of a rug pull)

The attacker can push unlimitedly to an array, that some function loop over this array. If increasing the array size enough, calling the function that does a loop over the array will always revert since there is a gas limit. This is an High Risk issue since those arrays are publicly allows to push items into them.

(https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/TokenManagerLinker.sol#L109)

    TokenManagerLinker.sol (L120): Unbounded loop on the array tokenManagers that can be publicly pushed by ['registerTokenManager']
    TokenManagerLinker.sol (L189): Unbounded loop on the array tokenManagers that can be publicly pushed by ['registerTokenManager']
    TokenManagerLinker.sol (L176): Unbounded loop on the array tokenManagers that can be publicly pushed by ['registerTokenManager']
    TokenManagerLinker.sol (L164): Unbounded loop on the array tokenManagers that can be publicly pushed by ['registerTokenManager']
    TokenManagerLinker.sol (L150): Unbounded loop on the array tokenManagers that can be publicly pushed by ['registerTokenManager']

Title: Anyone can withdraw others Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him

    CommunityPool.withdrawFunds

Title: Open TODOs Severity: Low Risk

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

    Open TODO in CommunityLocker.sol line 218 :         // TODO: uncomment when oracle finished

Title: Change transferFrom to transfer Severity: GAS

'transferFrom(address(this), , **)' could be replaced by the following more gas efficient 'transfer(, **)' This replacement is more gas efficient and improves the code quality.

    TokenManagerERC1155.sol, 345 : IERC1155Upgradeable(token).safeTransferFrom(address(this), receiver, id, amount, "");
    DepositBoxERC721.sol, 149 : IERC721Upgradeable(message.token).transferFrom(address(this), message.receiver, message.tokenId);
    TokenManagerERC721.sol, 244 : IERC721Upgradeable(token).transferFrom(address(this), receiver, tokenId);
    TokenManagerERC1155.sol, 401 : IERC1155Upgradeable(token).safeBatchTransferFrom(address(this), receiver, ids, amounts, "");
    DepositBoxERC721.sol, 192 : IERC721Upgradeable(erc721OnMainnet).transferFrom(address(this), receiver, tokenId);

Title: Unnecessary array boundaries check when loading an array element twice Severity: GAS

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:

    DepositBoxERC721.sol.initializeAllTokensForSchain - double load of tokens[i]
    TokenManagerLinker.sol.connectSchain - double load of tokenManagers[i]
    DepositBoxERC20.sol.initializeAllTokensForSchain - double load of tokens[i]

Title: State variables that could be set immutable Severity: GAS

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

    messageProxy in TokenManagerLinker.sol
    messageProxy in CommunityLocker.sol
    tokenManagerLinker in CommunityLocker.sol
    communityLocker in TokenManager.sol
    schainHash in CommunityLocker.sol
    linker in DepositBox.sol
    messageProxy in TokenManager.sol
    linkerAddress in TokenManagerLinker.sol
    contractManagerOfSkaleManager in SkaleManagerClient.sol
    communityPool in CommunityLocker.sol
    tokenManagerLinker in TokenManager.sol
    keyStorage in MessageProxyForSchain.sol

Title: Use bytes32 instead of string to save gas whenever possible Severity: GAS

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.


    TokenManagerLinker.sol (L41), string constant public MAINNET_NAME = "Mainnet";
    CommunityLocker.sol (L42), string constant public MAINNET_NAME = "Mainnet";
    TokenManager.sol (L44), string constant public MAINNET_NAME = "Mainnet";

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    MessageProxyForSchain.sol, postOutgoingMessage
    TokenManagerERC1155.sol, supportsInterface
    DepositBox.sol, initialize
    DepositBoxERC1155.sol, supportsInterface
    DepositBox.sol, isWhitelisted
    DepositBoxEth.sol, initialize
    Twin.sol, initialize
    DepositBoxERC721.sol, initialize
    MessageProxyForSchain.sol, removeConnectedChain

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    CommunityLocker.sol (L#174) - require( lastMessageTimeStamp[receiver] + timeLimitPerMessage < block.timestamp, "Trying to send messages too often" );

Title: Change if -> revert pattern to require Severity: GAS

Change if -> revert pattern to 'require' to save gas and improve code quality,
if (some_condition) {
        revert(revert_message)
}

to: require(!some_condition, revert_message)

In:

    Linker.sol, 117

Title: Consider inline the following functions to save gas Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    MessageProxyForSchain.sol, _getEtherbase, { return ETHERBASE; }
    MessageProxyForMainnet.sol, _getRegistryContracts, { return _registryContracts; }
    MessageProxyForSchain.sol, _getRegistryContracts, { return _registryContracts; }
    DepositBoxERC20.sol, _getErc20TotalSupply, { return erc20Token.totalSupply(); }
    TokenManagerERC20.sol, _getErc20TotalSupply, { return erc20Token.totalSupply(); }

Title: Unused imports Severity: GAS

In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    TokenManagerLinker.sol, line 26, import "../Messages.sol";
    FieldOperations.sol, line 26, import "./Precompiled.sol";

Title: Unnecessary index init Severity: GAS

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:

    Linker.sol, 100
    Linker.sol, 149
    TokenManagerLinker.sol, 151
    DepositBoxERC1155.sol, 444
    DepositBoxERC20.sol, 76
    MessageProxyForMainnet.sol, 235
    MessageProxyForSchain.sol, 118
    DepositBoxERC1155.sol, 275
    TokenManagerLinker.sol, 192
    DepositBoxERC1155.sol, 459
    DepositBoxERC1155.sol, 79
    MessageProxyForMainnet.sol, 118
    TokenManagerERC1155.sol, 500
    TokenManagerERC1155.sol, 514
    TokenManagerLinker.sol, 166
    MessageProxyForSchain.sol, 222
    DepositBoxERC721.sol, 76
    Linker.sol, 175

Title: Use != 0 instead of > 0 Severity: GAS

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

    TokenManagerERC20.sol, 250: change 'balance > 0' to 'balance != 0'
    DepositBoxERC20.sol, 157: change 'balance > 0' to 'balance != 0'
    CommunityPool.sol, 124: change 'gas > 0' to 'gas != 0'
    TokenManagerERC20.sol, 280: change 'balance > 0' to 'balance != 0'

Title: Short the following require messages Severity: GAS

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:

    Solidity file: CommunityLocker.sol, In line 174, Require message length to shorten: 33, The message: Trying to send messages too often
    Solidity file: MessageProxyForMainnet.sol, In line 327, Require message length to shorten: 33, The message: Sender contract is not registered
    Solidity file: CommunityLocker.sol, In line 144, Require message length to shorten: 33, The message: Source chain name must be Mainnet
    Solidity file: MessageProxyForMainnet.sol, In line 217, Require message length to shorten: 34, The message: Schain wallet has not enough funds
    Solidity file: MessageProxyForMainnet.sol, In line 320, Require message length to shorten: 34, The message: Schain id can not be equal Mainnet

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    MessageProxyForSchain.removeExtraContract (chainName)
    ERC721OnChain.constructor (contractName)
    MessageProxyForSchain.removeConnectedChain (chainName)
    TokenManager.initializeTokenManager (newSchainName)
    TokenManagerERC721.initialize (newChainName)
    ERC20OnChain.constructor (contractName)
    MessageProxyForMainnet.registerExtraContract (schainName)
    ERC721OnChain.constructor (contractSymbol)

Title: Inline one time use functions Severity: GAS

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

    TokenManagerERC20.sol, _receiveERC20
    TokenManager.sol, _checkSender
    TokenManagerERC1155.sol, _receiveERC1155Batch
    MessageProxyForSchain.sol, _topUpBalance
    TokenManagerERC721.sol, _saveTransferredAmount
    DepositBoxEth.sol, _saveTransferredAmount
    TokenManagerERC721.sol, _addERC721ForSchain
    DepositBoxERC721.sol, _receiveERC721

Title: Unused inheritance Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    TokenManagerERC1155.sol; the inherited contracts ERC1155ReceiverUpgradeableWithoutGap not used

Title: Caching array length can save gas Severity: 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++) { ... }


    Linker.sol, schainContracts, 100
    DepositBoxERC1155.sol, ids, 444
    MessageProxyForMainnet.sol, messages, 235
    DepositBoxERC1155.sol, ids, 275
    DepositBoxERC1155.sol, tokens, 79
    TokenManagerERC1155.sol, ids, 500
    DepositBoxERC20.sol, tokens, 76
    DepositBoxERC721.sol, tokens, 76
    MessageProxyForSchain.sol, messages, 222
    TokenManagerLinker.sol, tokenManagers, 151
    DepositBoxERC1155.sol, ids, 459
    MessageProxyForSchain.sol, contracts, 118
    TokenManagerERC1155.sol, ids, 514
    MessageProxyForMainnet.sol, contracts, 118

Title: Unused state variables Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    TokenManagerERC20.sol, deprecatedClonesErc20
    MessageProxyForSchain.sol, _idxHead
    TokenManagerERC721.sol, deprecatedClonesErc721
    TokenManagerERC1155.sol, deprecatedClonesErc1155
    TokenManagerLinker.sol, MAINNET_HASH

Title: Unnecessary cast Severity: Gas

    address TokenManagerERC20.sol._exit - unnecessary casting address(contractOnMainChain)

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    ERC721OnChain.sol, _burn
    ERC1155ReceiverUpgradeableWithoutGap.sol, __ERC1155Receiver_init_unchained
    Precompiled.sol, bn256Pairing
    ERC1155ReceiverUpgradeableWithoutGap.sol, __ERC1155Receiver_init
    MessageProxyForMainnet.sol, _checkSchainBalance
    MessageProxyForMainnet.sol, _verifyMessages
    MessageProxyForSchain.sol, _getRegistryContracts
    TokenManager.sol, _checkSender
    MessageProxyForSchain.sol, _verifyMessages
    MessageProxyForMainnet.sol, _getRegistryContracts
    MessageProxyForSchain.sol, _getEtherbase
    MessageProxyForMainnet.sol, _authorizeOutgoingMessageSender
    SkaleVerifier.sol, verify

Title: Prefix increments are cheaper than postfix increments Severity: GAS

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:

    change to prefix increment and unchecked: TokenManagerLinker.sol, index, 122
    change to prefix increment and unchecked: DepositBoxERC1155.sol, i, 79
    change to prefix increment and unchecked: DepositBoxERC1155.sol, i, 275
    change to prefix increment and unchecked: TokenManagerLinker.sol, i, 166
    change to prefix increment and unchecked: DepositBoxERC20.sol, i, 76
    change to prefix increment and unchecked: DepositBoxERC20.sol, i, 276
    change to prefix increment and unchecked: DepositBoxERC721.sol, i, 260
    change to prefix increment and unchecked: MessageProxyForMainnet.sol, i, 235
    change to prefix increment and unchecked: DepositBoxERC1155.sol, i, 398
    change to prefix increment and unchecked: Linker.sol, i, 100
    change to prefix increment and unchecked: DepositBoxERC721.sol, i, 76
    change to prefix increment and unchecked: MessageProxyForMainnet.sol, i, 118
    change to prefix increment and unchecked: DepositBoxERC1155.sol, i, 444
    change to prefix increment and unchecked: Linker.sol, i, 175
    change to prefix increment and unchecked: Linker.sol, i, 149
    change to prefix increment and unchecked: MessageProxyForSchain.sol, i, 222
    change to prefix increment and unchecked: TokenManagerLinker.sol, i, 192
    change to prefix increment and unchecked: TokenManagerLinker.sol, index, 178
    change to prefix increment and unchecked: TokenManagerERC1155.sol, i, 500
    change to prefix increment and unchecked: TokenManagerERC1155.sol, i, 514
    change to prefix increment and unchecked: TokenManagerLinker.sol, i, 151
    change to prefix increment and unchecked: MessageProxyForSchain.sol, i, 118
    change to prefix increment and unchecked: DepositBoxERC1155.sol, i, 459

Title: Unnecessary functions Severity: GAS

The following functions are not used at all. Therefore you can remove them to save deployment gas and improve code clearness.


    Precompiled.sol, bn256Pairing
    ERC1155ReceiverUpgradeableWithoutGap.sol, __ERC1155Receiver_init
    SkaleVerifier.sol, verify

messageProxy in TokenManagerLinker.sol -> no as contract doesn't have a constructor
messageProxy in CommunityLocker.sol -> same
tokenManagerLinker in CommunityLocker.sol -> same
communityLocker in TokenManager.sol -> same
schainHash in CommunityLocker.sol -> same