code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyForMainnet.sol#L246-L252

Vulnerability details

The contest code does a good job of preventing users from withdrawing too quickly or attempting to do things without funds for gas. The nodes themselves however are not as well-secured.

Impact

By monitoring the values of headerMessageGasCost and messageGasCost and comparing them to the actual gas cost, a malicious node can choose the most opportune time to execute an attack that drains each of the chains it is in charge of's chain wallets. By sending messages where the refund is larger than the input costs, the node is able to take money from the chain wallets. The node can layer on plausible deniability by not just doing the attack for transactions it is in charge of, but by also doing it for the other nodes that are also in charge of each chain at different time slots.

Proof of Concept

The node can create a lot of separate users on multiple chains then withdraw all but the last bit of money in each user's chain's community pool. When the gas conditions are right the node can create a lot of small withdrawals in large batch transactions that take out the last bits of money for each user on each chain. Since each user doesn't have much in its account, the refund is taken from each schain wallet.

The small withdrawal will lock the user's account which will trigger the Messages.encodeLockUserMessage to be sent...

    function withdrawFunds(string calldata schainName, uint amount) external override {
        bytes32 schainHash = keccak256(abi.encodePacked(schainName));
        require(amount <= _userWallets[msg.sender][schainHash], "Balance is too low");
        require(!messageProxy.messageInProgress(), "Message is in progress");
        _userWallets[msg.sender][schainHash] = _userWallets[msg.sender][schainHash] - amount;
        if (
            !_balanceIsSufficient(schainHash, msg.sender, 0) &&
            activeUsers[msg.sender][schainHash]
        ) {
            activeUsers[msg.sender][schainHash] = false;
            messageProxy.postOutgoingMessage(
                schainHash,
                schainLinks[schainHash],
                Messages.encodeLockUserMessage(msg.sender)
            );
        }
        payable(msg.sender).sendValue(amount);
    }

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/CommunityPool.sol#L169-L186

...which will be reimbursed from the schain wallet

            } else {
                _callReceiverContract(fromSchainHash, messages[i], startingCounter + i);
                notReimbursedGas += gasTotal - gasleft() + additionalGasPerMessage;
            }
        }
        connectedChains[fromSchainHash].incomingMessageCounter += messages.length;
        communityPool.refundGasBySchainWallet(fromSchainHash, payable(msg.sender), notReimbursedGas);

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyForMainnet.sol#L246-L252

There may be other ways to trigger larger messages that require a larger refund, but this example is proof of concept enough. Most of the documentation talks about slashing validator nodes if they do something visible like double-spend. This however is a case where cheating is not identifiable by on-chain metrics if the node is determined to remain anonymous while it's attacking. The system requirements for being a validator are also not very onerous so it's relatively easy for an attacker to become a validator (once this project is more widely used and needs to onboard more validators).

Tools Used

Code inspection

Recommended Mitigation Steps

Rate-limit schain wallet refunds on both a per-chain and a global level, and have a dust limit under which funds cannot be withdrawn.

cstrangedk commented 2 years ago

Not possible as sending a message to Mainnet requires a BLS threshold signature of at least 2/3 * 16 randomly selected nodes that support the SKALE Chain conducting the transfer. See https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyForMainnet.sol#L224-L231

GalloDaSballo commented 2 years ago

Have to agree with the sponsor, this would require 2/3 of signers to collude to arbitrage the gas cost at the risk of being slashed with the added risk of needing the colluders to be selected as signers

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