SKALE contest - cmichel's results

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L223

Vulnerability details

Impact

The postIncomingMessages function calls _callReceiverContract(fromChainHash, messages[i], startingCounter + 1) which gives control to a contract that is potentially attacker controlled before updating the incomingMessageCounter.

for (uint256 i = 0; i < messages.length; i++) {
    // @audit re-entrant, can submit same postIncomingMessages again
    _callReceiverContract(fromChainHash, messages[i], startingCounter + 1);
}
connectedChains[fromChainHash].incomingMessageCounter += messages.length;

The attacker can re-enter into the postIncomingMessages function and submit the same messages again, creating a replay attack. Note that the startingCounter is the way how messages are prevented from replay attacks here, there are no further nonces.

POC

Attacker can submit two cross-chain messages to be executed:

1. Transfer 1000 USDC
2. A call to their attacker-controlled contract, could be masked as a token contract that allows re-entrance on transfer.

Some node submits the postIncomingMessages(params) transaction, transfers 1000 USDC, then calls the attackers contract, who can themself call postIncomingMessages(params) again, receive 1000 USDC a second time, and stop the recursion.

Recommended Mitigation Steps

Add a messageInProgressLocker modifier to postIncomingMessages as was done in MessageProxyForMainnet.

Lines of code

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

Vulnerability details

Impact

A connected chain can be removed which will make all pending messages fail. If the chain is reinitialized again at some point, its incomingMessageCounter will start at zero and allow replay attacks of all previous messages.

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];
}

function _addConnectedChain(bytes32 schainHash) internal onlyChainConnector {
    require(!connectedChains[schainHash].inited,"Chain is already connected");
    connectedChains[schainHash] = ConnectedChainInfo({
        // @audit counters reset to zero
        incomingMessageCounter: 0,
        outgoingMessageCounter: 0,
        inited: true
    });
}

Recommended Mitigation Steps

Not sure if a chain should ever be removed. If it's removed once, it should be permanently disabled.

Lines of code

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

Vulnerability details

Impact

Some mainnet tokens (like USDT) don't correctly implement the EIP20 standard and their transfer/transferFrom function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

Non-safe transfers are used in:

• DepositBoxERC20.depositERC20

Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value.

Recommended Mitigation Steps

For the mainnet contracts, we recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Lines of code

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

Vulnerability details

Impact

Some ERC20 tokens make modifications to their ERC20's transfer or balanceOf functions. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom(). Others are rebasing tokens that increase in value over time like Aave's aTokens (balanceOf changes over time).

The schains deploy a mirror standard ERC20 contract for all token transfers from other chains. This standard contract does not have these transfer modifications which leads to issues:

• Fee-on-transfer mainnet token transfers are reported with the pre-fee amount. It deploys a standard ERC20 chain on the receiving chain and mints this pre-fee amount. When trying to retrieve the pre-fee amount by doing the reverse cross-chain transfer, the contract on the receiving side will be unable to pay out the pre-fee amount as its balance is the post-fee amount.

Recommended Mitigation Steps

It seems like the issue with fee-on-transfer tokens could be prevented by only minting the post-fee amount on the receiving chain. Consider computing the actual received token amounts from the transferFrom call and using this amount in the messageProxy.postOutgoingMessage call.

A similar issue exists on schains where no clone ERC20 is deployed but the mainnet equivalent token is used which could be a fee-on-transfer token.

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/tokens/ERC721OnChain.sol#L73

Vulnerability details

Impact

In the ERC721OnChain implementation the token owner can set the token's URI using setTokenURI. Usually, this is token URI points to data defining the NFT (attributes, images, etc.). It's usually set by the contract owner. A user that owns an NFT can just spoof any other NFT data by changing the token URI to any of the other NFTs.

Recommended Mitigation Steps

Disallow the owner of an NFT to change its token URI

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L223 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L425

Vulnerability details

Impact

The postIncomingMessages function iterates over all messages and calls _callReceiverContract with a counter value of startingCounter + 1 for all of them. It should be startingCounter + i.

If a call fails the PostMessageError event is emitted with startingCounter + 1 instead of the actual counter. I'm not sure how the nodes handle failed messages but I assume they would undo/refund the original transaction based on these PostMessageError events that use the counter to identify a message.

This could then lead to loss of funds. Imagine I post 2 messages:

1. Something that always reverts
2. Transfer of 1000 USDC

The first message will revert but emit that the second message reverted. I receive the 1000 USDC on the receiving chain and also get a refund of 1000 USDC, stealing funds.

Recommended Mitigation Steps

- _callReceiverContract(fromChainHash, messages[i], startingCounter + 1);
+ _callReceiverContract(fromChainHash, messages[i], startingCounter + i);