code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxEth.sol#L138-L142 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L196-L200 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC721.sol#L183-L187 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol#L261-L271

Vulnerability details

Impact

Once a chain has been killed the chain owner is able to call getFunds() on each of the deposit boxes and transfer funds/tokens wherever he/she wishes

Even if the owner is benevolent the fact that there is a rug vector available may negatively impact the protocol's reputation. See this example where a similar finding has been flagged as a high-severity issue. I've downgraded these instances to be a medium since it requires cooperation of the IMA mainnet admin.

Proof of Concept

    function getFunds(string calldata schainName, address payable receiver, uint amount)
        external
        override
        onlySchainOwner(schainName)
        whenKilled(keccak256(abi.encodePacked(schainName)))

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxEth.sol#L138-L142

    function getFunds(string calldata schainName, address erc20OnMainnet, address receiver, uint amount)
        external
        override
        onlySchainOwner(schainName)
        whenKilled(keccak256(abi.encodePacked(schainName)))

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L196-L200

    function getFunds(string calldata schainName, address erc721OnMainnet, address receiver, uint tokenId)
        external
        override
        onlySchainOwner(schainName)
        whenKilled(keccak256(abi.encodePacked(schainName)))

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC721.sol#L183-L187

    function getFunds(
        string calldata schainName,
        address erc1155OnMainnet,
        address receiver,
        uint256[] memory ids,
        uint256[] memory amounts
    )
        external
        override
        onlySchainOwner(schainName)
        whenKilled(keccak256(abi.encodePacked(schainName)))

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol#L261-L271

Tools Used

Code inspection

Recommended Mitigation Steps

Add a long time lock for killing so users have plenty of time to get their funds out before a kill

DimaStebaev commented 2 years ago

This is not a vulnerability. We explicitly added this functionality to prevent loosing of funds in case of technical problems with SKALE chain. It requires SKALE chain owner and SKALE foundation cooperation to run this mechanism. It is going to be removed eventually after some time of stable work.

GalloDaSballo commented 2 years ago

While I empathize with the sponsor's side that the function is meant as a security mechanism, it does allow the chainOwner to pull all funds and transfer them to their own wallet.

Because this is contingent on Admin Privilege, I believe Medium Severity to be appropriate

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