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

0 stars 0 forks source link

Schain owner dictate fund usage after kill #54

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

After a schain is killed by both the owner and the IMA admin, schain admin can control all the fund using e.g. DepositBoxERC20.getFunds functions. This pose a significant centralization risk after the schain is killed.

Proof of Concept

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

    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

Require both the owner and IMA admin on the fund distribution process.

cstrangedk commented 2 years ago

Issue here incorrectly assumes that the owner is a centralized authority. SKALE Chain owners may be a DAO, multisig, or single owner - the IMA bridge is agnostic to, and cannot control the governance structure adopted in each SKALE Chain. The responsibility and process conducted after kill is executed rests on the Owner and whatever governance structure is adopted.

GalloDaSballo commented 2 years ago

Dup of #76