QA Report

[code423n4]

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

[DimaStebaev]

2.

• SkaleVerifier.sol (L108) hash might be 0) - there is no division by hash
• MessageProxyForMainnet.sol (L232) messages might be 0) - if messages is empty it is no be signed by supermajority of nodes and computations is not be performed
• SkaleVerifier.sol (L108) hashA might be 0) - there is no division by hashA
• SkaleVerifier.sol (L108) hashB might be 0) - there is no division by hashB
• CommunityPool.sol (L111) gas might be 0) - tx.gasprice can't be 0 because of baseFee.
  4. Because hashB can be equal to 2^256 - 1 it can't be multiplied by 2 and p / 2 is assumed to be floor(p / 2)
  5. Values are validated on nodes side and are singed by threshold signature.
  6. contracts exist in genesis block of SKALE chain and are not deployed. Initializers are used only for testing.
  7. Only SKALE chain owner can add token managers. It is assumed to be trusted party.
  8. users can withdraw deposited ETH any time by design.

[GalloDaSballo]

Title: Duplicates in array

Valid

Title: Div by 0

Disagree with finding, per the sponsor reply and per lack of nuance.

Title: Never used parameters

Non-critical

Title: Mult instead div in compares

Per the sponsor reply, invalid

Title: transfer return value of a general ERC20 is ignored

Agree

Title: Not verified input

Agree

Title: Init frontrun

Agree

Title: Unbounded loop on array can lead to DoS

Disagree it can lead to dos as it's permission

Title: Anyone can withdraw others

Disagree per the sponsor reply

Title: Open TODOs

Non-critical

Overall a heartless report