If SChain is Removed Before `kill()` and `getFunds()` all Tokens are Locked in the Bridge

[code423n4]

Lines of code

https://github.com/skalenetwork/skale-manager/blob/6827f3c8918424641647f20700232713626be828/contracts/SchainsInternal.sol#L206-L234 https://github.com/skalenetwork/skale-manager/blob/6827f3c8918424641647f20700232713626be828/contracts/SchainsInternal.sol#L522-L524 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/SkaleManagerClient.sol#L70-L74 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L196-L209

Vulnerability details

Impact

If the SChain is removed before all the funds are withdrawn from the bridge they will be permanently locked in the bridge.

When a SChain is removed in by the SKALE protocol via the function SchainsInternal.removeSchain() the data including the owner will be deleted. As a result the function SchainsInternal.getSchainOwner() will always return address(0).

Since getFunds() in each of the deposit boxes can only be called by the SChain owner (i.e. has the modifier onlySchainOwner) and the owner address is now address(0) getFunds() will always revert. The impact is that any funds associated with this SChain are now locked in the bridge.

Proof of Concept

SchainsInternal.sol#L206-L234

    function removeSchain(bytes32 schainHash, address from)
        external
        override
        allow("Schains")
        schainExists(schainHash)
    {
        isSchainActive[schainHash] = false;
        uint length = schainIndexes[from].length;
        uint index = schains[schainHash].indexInOwnerList;
        if (index != length - 1) {
            bytes32 lastSchainHash = schainIndexes[from][length - 1];
            schains[lastSchainHash].indexInOwnerList = index;
            schainIndexes[from][index] = lastSchainHash;
        }
        schainIndexes[from].pop();

        // TODO:
        // optimize
        for (uint i = 0; i + 1 < schainsAtSystem.length; i++) {
            if (schainsAtSystem[i] == schainHash) {
                schainsAtSystem[i] = schainsAtSystem[schainsAtSystem.length - 1];
                break;
            }
        }
        schainsAtSystem.pop();

        delete schains[schainHash];
        numberOfSchains--;
    }

SchainsInternal.sol#L522-L524

    function getSchainOwner(bytes32 schainHash) external view override schainExists(schainHash) returns (address) {
        return schains[schainHash].owner;
    }

SkaleManagerClient.sol#L70-L74

    function isSchainOwner(address sender, bytes32 schainHash) public view override returns (bool) {
        address skaleChainsInternal = contractManagerOfSkaleManager.getContract("SchainsInternal");
        return ISchainsInternal(skaleChainsInternal).isOwnerAddress(sender, schainHash);
    }
}

DepositBoxERC20.sol#L196-L209

    function getFunds(string calldata schainName, address erc20OnMainnet, address receiver, uint amount)
        external
        override
        onlySchainOwner(schainName)
        whenKilled(keccak256(abi.encodePacked(schainName)))
    {
        bytes32 schainHash = keccak256(abi.encodePacked(schainName));
        require(transferredAmount[schainHash][erc20OnMainnet] >= amount, "Incorrect amount");
        _removeTransferredAmount(schainHash, erc20OnMainnet, amount);
        require(
            ERC20Upgradeable(erc20OnMainnet).transfer(receiver, amount),
            "Transfer was failed"
        );
    }

Recommended Mitigation Steps

Consider allowing a fallback option, such as the DEFAULT_ADMIN_ROLE of the boxes to be allowed to call getFunds() if SchainsInternal.getSchainOwner() returns address(0) and there is still balance in the deposit box for this SChain.

[liveactionllama]

Adding comment per warden's request:

I didn't realise until the recent discussion that we should add each occurrence of a bug individually, even if the root cause is the same. It's mentioned in the issue that it works for "each of the deposit boxes". The location of this bug is for each of the following getFunds() functions:

• DepositBoxERC1155
• DepositBoxERC20
• DepositBoxERC721
• DepositBoxEth

[DimaStebaev]

Kill functionality is temporary and will be removed eventually. It's needed for emergency cases. We hope it would be never used. Typical way to stop SKALE chain - first announce it, then allow to withdraw all funds and only then terminate it.

[GalloDaSballo]

Given that the finding is based on Admin Privilege, I believe Medium Severity to be more appropriate

[GalloDaSballo]

Dup of #71