code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Users on schain may burn their tokens and this will leave the counterpart on mainnet permanently trapped unless the schain is killed. This can be generalised to other custom implementations with any type of burn functionality that can be called directly on the clone contract, without bridging tokens back to mainnet.

Proof of Concept (ERC20)

The burn function on ERC20OnChain is implemented inside the inherited ERC20BurnableUpgradeable contract

    function burn(uint256 amount) public virtual {
        _burn(_msgSender(), amount);
    }

This function has no access control so that anyone can burn their own tokens. Once the tokens are burned, they cannot be bridged over to mainnet since in _exit on TokenManagerERC20.sol the amount needs to be less than the balance.

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

 require(contractOnSchain.balanceOf(msg.sender) >= amount, "Insufficient funds");

This means that there will be tokens permanently trapped in transferredAmount[schainHash][token] that corresponds to the burned amount unless the schain is killed and the owner of schain then will be able to getFunds and return the token.

Tools Used

Recommended Mitigation Steps

Wrap burn function with access control on ERC20OnChain, or even better disable burn called directly on the clone contract on schain.

cstrangedk commented 2 years ago

Similar disputed issue of #17, #38, here applies to ERC20OnChain token clone and the burner role..

In production with automatic deploy mode enabled, ERC20OnChain is deployed when token is received in DepositBox and subsequent postMessage()._sendERC20() method is executed... MINTER_ROLE and burner address is set to TokenManagerERC20, which is responsible for minting and burning token clones in response to Deposits in DepositBoxes. When automatic deploy is not abled, the SKALE chain owner is responsible for manually adding tokens and assigning the appropriate MINT and burn addresses.

Here TokenManagerERC20 mints tokens in response to a deposit https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L231

and here burns the token in response to an exit: https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L306

further defined in the docs

https://docs.skale.network/ima/1.2.x/flows#_manual_setup_steps https://docs.skale.network/ima/1.2.x/flows#_automatic_setup_steps

GalloDaSballo commented 2 years ago

I don't think this is a duplicate.

Yes people can burn their own tokens, that's a feature they can elect to choose just like sending tokens to the 0x or sending a tip to entreprenerd.eth

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