search

code-423n4 / 2022-02-skale-findings

0 stars 0 forks source link

QA Report #45

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Low Severity

[L01] Outdated version of inherited contracts and upgrade library

@openzeppelin/contracts-upgradeable   4.3.2  -> 4.5.1 
@openzeppelin/hardhat-upgrades        1.10.0 -> 1.14.0

[L02] Not calling inherited function directly

For instance https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/SkaleManagerClient.sol#L62

AccessControlEnumerableUpgradeable.__AccessControlEnumerable_init();

is not necessary. Just use __AccessControlEnumerable_init();. It compiles and all tests passed.

[L03] Require messages not indicative of the contracts

Almost all require message has no indication of which contract it is coming from. It is a good practice for debugging and monitoring purposes to mark the contract that reverted.

For example, https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L188 the revert message could be "MessageProxy: Extra contract is not registered". Many other instances are also present in almost all contracts by globally search require.

[L04] Use initializer instead of onlyInitializing

In OpenZeppline-contracts-upgradeable v4.5 initializer has a new modifier https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/20380ee699406211ae88c4edd11b0af87c8abdd6/contracts/proxy/utils/Initializable.sol#L72

    modifier onlyInitializing() {
        require(_initializing, "Initializable: contract is not initializing");
        _;
    }

to mark and protect the initializing functions that should not be called directly called.

Instances such as

[L05] Inconsistent use of schainHash or schainName when referencing a schain

There is inconsistency in referring to a schain, sometimes by bytes32 hash, sometimes by string name in a single contract.

For example, in SkaleManagerClient,

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/SkaleManagerClient.sol#L70-L73 uses schainNash and https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/SkaleManagerClient.sol#L42-L48 uses schainName.

Bytes32 should be preferred over string for gas-saving reasons.

[L06] Schain owner cannot add ERC20 token manually as specified in the doc

The specification in the doc is inconsistent with the code. For example in https://docs.skale.network/ima/1.2.x/managing-erc20 and the diagram https://docs.skale.network/ima/1.2.x/flows#_manual_setup_steps is not consistent with the code.

Minter role in ERC20 is granted automatically either to TokenManagerERC20 or to the deployer in the constructor.

[L07] Outdated Solidity Version

Solidity version 0.8.6 is used throughout. Many updates have been applied to the compiler. A more recent version such as 0.8.10 would work better. The latest version is 0.8.11.

[L08] Use OnlyRole() modifier from inherited contract AccessControlUpgradeable

Many instances can be simplified using the OnlyRole modifier.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/20380ee699406211ae88c4edd11b0af87c8abdd6/contracts/access/AccessControlUpgradeable.sol#L75-L78

For instance, https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L237 and https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/tokens/ERC20OnChain.sol#L60-L61 and many other role-based require statements.

Informational

[N01] NatSpec on Events and Functions

Many functions and events do not have @param and @return labelled in Natspec, for instance

[N02] initialize functions positioned too much down the contract

initialize function is a special feature in upgradeability patterns and often important and prone to grave consequences too. It is a good practice to put them at the beginning of the contract, like those in the OpenZeppelin-contracts-upgradeable library.

[N03] Repeated code

Repeated unimplemented function postIncomingMessages() in both

and "@skalenetwork/ima-interfaces/IMessageProxy.sol" .

One can remove the one in MessageProxy.sol without affecting any test results.

Gas Saving

[G01] public functions can be external

Here are two example instances

DimaStebaev commented 2 years ago

L06: If SKALE chain owner adds custom ERC20 it must allow TokenManager to minting it. Custom token can have custom logic of minting N02: In our code style we always put public functions after external G01: They are called inside smart contract in child contracts

GalloDaSballo commented 2 years ago

[L01] Outdated version of inherited contracts and upgrade library

Would like to see explanation of vulns like other wardens, downgrading to non-critical

[L02] Not calling inherited function directly

Also non-critical as it's just explicit inheritance

[L03] Require messages not indicative of the contracts

Non-critical as if all messages are unique you don't need the contract, also the revert trace will show you the contract as well

[L04] Use initializer instead of onlyInitializing

I think this is an interesting and valid finding

[L05] Inconsistent use of schainHash or schainName when referencing a schain

Agree but Non-critical

[L06] Schain owner cannot add ERC20 token manually as specified in the doc

Disagree with finding per-sponsor reply

[L07] Outdated Solidity Version

Will leave as valid although the warden has missed some bugs from 0.8.6

[L08] Use OnlyRole() modifier from inherited contract AccessControlUpgradeable

Agree, but non-critical as it's just a refactoring

[N01] NatSpec on Events and Functions

Agree

[N02] initialize functions positioned too much down the contract

Per the sponsor's styleguide, consistency > dogma

[N03] Repeated code

I believe this is a test function but finding is valid nonetheless

I really like this report as it's short and sweet and has some interesting finds, good job!