code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/tokens/ERC20OnChain.sol#L41

Vulnerability details

Impact

MINTER_ROLE will not be assigned in the context of the proxy, thus, not having the privilege of minting. ERC20BurnableUpgradeable will not be initialised either in the proxy context. This may break the mechanism of bridging.

Proof of Concept

The constructor in an Upgradeable contract such as ERC20OnChain will only be run once during deployment in the implementation context, but not executed in the context of its proxy state. The logic in the constructor will not be part of the bytecode.

Tools Used

Recommended Mitigation Steps

use initialize() instead of a constructor.

DimaStebaev commented 2 years ago

ERC20OnChain is inherited from upgradeable ERC20 implementation but is not upgradeable. It is deployed as a regular smart contract without proxy and all fields are initialized in the constructor.

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L231

GalloDaSballo commented 2 years ago

The sponsor chose to use the initialize pattern on non-upgradeable contracts, that by itself is not a vulnerability.

However this means that finding related to front-run initialization may be valid in lack of a router that deploys and initializes in one go

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