search

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

0 stars 0 forks source link

Loss of pending messages (if any) in case removeConnectedChain is called #63

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L313-L317

Vulnerability details

Impact

If there are any unprocessed messages to be executed or processed, while removeConnectedChain is called, then they may be stuck from getting processed on the other end. If these messages have transactions for any token transfer then it will get stuck or lost.

Proof of Concept

Contract : MessageProxy.sol Line : 313 

    function removeConnectedChain(string memory schainName) public virtual override onlyChainConnector {
        bytes32 schainHash = keccak256(abi.encodePacked(schainName));
        require(connectedChains[schainHash].inited, "Chain is not initialized");
        delete connectedChains[schainHash];
    }

Recommended Mitigation Steps

Check if there are any pending or unprocessed messages while removeConnectedChain is called and revert in that case. Better to implement some functionality like pause just locally for the chain to be removed, before the actual removeConnectedChain is called.

DimaStebaev commented 2 years ago

It duplicates #57

GalloDaSballo commented 2 years ago

I don't believe this to be a duplicate.

I think the finding is valid in that because of the synchronicity of broadcasting messages, the chain could be removed before it receives all messages.

This is a risk that end users do face when interacting with the system and the only use case I could think of would be for a malicious admin to deny certain operations.

That said I don't believe there's any easy solution as this would have to be addressed at the meta level.

I do think the finding is valid and of medium severity