Centralisation Risk: `TokenManager` Gives Unnecessary Permissions to The Default Admin Through `changeDepositBoxAddress()` Which May Cause The Bridge to Get Stuck

[code423n4]

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/TokenManager.sol#L223-L228

Vulnerability details

Impact

The function changeDepositBoxAddress() allows the DEFAULT_ADMIN_ROLE to change the depositBox associated with a TokenManager. If the newDepositBox is incorrectly set (either accidentally or maliciously) it will cause the bridge to become stuck.

For example if we have a TokenManagerERC20.sol and the admin sets the newDepositBox = address(1). Then when a user exits the SChain they will post a message with messageReceiver = depositBox = address(1) as seen in the following.

        messageProxy.postOutgoingMessage(
            chainHash,
            messageReceiver,
            data
        );

Due to another bug in that __callReceiverContract() will be called even if the destinationContract is not registered. Hence, when the message is posted to the MessageProxy on the main chain by a validator it will call _callReceiverContract(), which has the following code.

Noting desinationContract = messageReceiver = address(1).

try IMessageReceiver(message.destinationContract).postMessage{gas: gasLimit}(
            schainHash,
            message.sender,
            message.data
        ) returns (address receiver) {
            return receiver;
        }

The result will be that since address(1) does not have any bytecode the returndata will be of length zero and hence receiver = abi.decode(returndata, (address)) will revert and therefore the entire transaction will revert when processing postIncomingMessages() for this SChain. Since we cannot process this message the bridge will become stuck.

Proof of Concept

    function changeDepositBoxAddress(address newDepositBox) external {
        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "DEFAULT_ADMIN_ROLE is required");
        require(newDepositBox != address(0), "DepositBox address has to be set");
        emit DepositBoxWasChanged(depositBox, newDepositBox);
        depositBox = newDepositBox;
    }

Recommended Mitigation Steps

This issue may be mitigated by removing the function changeDepositBoxAddress() and thus creating a permanent 1:1 relationship between a DepositBox and TokenManager that is set in initializeTokenManager(). Therefore, prevent accidental or malicious misuse by the admin.

If a DepositBox needs to be replaced (redeployed) on the main chain then it is reasonable to redeploy the TokenManager on the SChain.

p.s. a different recommendation for the issue of try-catch reverting is using a low level call instead of using a try catch. Note: the if(!success) {...} can be modified to emit the error logs.

        (bool success, bytes memory data) = message.destinationContract.call{value: msg.value, gas: gasLimit}(
            abi.encodeWithSignature("postMessage(bytes32,address,bytes)", schainHash, message.sender, message.data)
        );
        if (!success) { return address(0); }
        if (data.length < 20) { return address(0); }
        return abi.decode(data, (address));

[DimaStebaev]

This function can be called only by SKALE chain owner. Besides that SKALE chain owner has enough power to spoof users in other ways. If it accidentally or maliciously break IMA only it affects only it's own chain and it can be restored using the same function.

[GalloDaSballo]

Dup of #35 from same warden, I agree with med severity but am marking invalid as it's dup sub