SKALE contest - gzeon's results

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.

Lines of code

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

Vulnerability details

Impact

The transfered amount is saved without checking the actual amount of token received after the transfer.

Proof of Concept

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

        _saveTransferredAmount(schainHash, erc20OnMainnet, amount);
        require(
            ERC20Upgradeable(erc20OnMainnet).transferFrom(
                msg.sender,
                address(this),
                amount
            ),
            "Transfer was failed"
        );

Recommended Mitigation Steps

Check before and after balance

Schain owner can censor user by not distributing sFUEL

SKALE chains operate in a gas-free environment using a native gas token called sFUEL. sFUEL has no economic value and is allocated from the SKALE chain owner

It is possible for schain owner to censor user / freeze user fund by not distributing enough sFUEL

setMinTransactionGas lack minimum value

Since the base Tx gas cost is 21000, some global minimum can be hardcoded to avoid misconfiguration https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/CommunityPool.sol#196

_balanceIsSufficient use current tx.gasprice but the actual gasprice node will pay is unknown

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

Upgrade Solidity Version

Recommended to upgrade to latest Solidity 0.8.12

Unresolved TODO

contracts/schain/CommunityLocker.sol:219:        // TODO: uncomment when oracle finished

Out-of-order encode-decode function

Some of the decode function is not adjacent to its encode counterpart, which make reading the code a bit annoying (e.g. encodeTransferErc20Message and decodeTransferErc20Message) https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/Messages.sol

depositERC20 lack input validation

DepositBoxERC20.depositERC20() did not check if amount!=0 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol

> 0 is less efficient than != 0 for uint in require condition

Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxEth.sol#L122

require(approveTransfers[msg.sender] > 0, "User has insufficient ETH");

Reuse value from storage

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

        uint userWalletBal = _userWallets[user][schainHash];
        if (amount > userWalletBal) {
            amount = userWalletBal;
            _userWallets[user][schainHash] = 0;
        }else{
            _userWallets[user][schainHash] = userWalletBal - amount;
        }

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

        uint userWalletBal = _userWallets[msg.sender][schainHash];
        require(amount <= userWalletBal, "Balance is too low");
        require(!messageProxy.messageInProgress(), "Message is in progress");
        _userWallets[msg.sender][schainHash] = userWalletBal - amount;

Precalc keeack constant

The keccak256(abi.encodePacked("Mainnet")) can be precalculated as constant https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBox.sol

Save gas by using schainName hash as input directly

It is a bit wasteful to use string schainName as input and hash it repeatedly onchain

Optimistic erc20 transfer

Can save some gas by skiping the balance check and let it revert during the transfer https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L157

require(ERC20Upgradeable(message.token).balanceOf(address(this)) >= message.amount, "Not enough money");