SKALE contest - rfa's results

1. No parent call for CommunityPool and DepositBox

    function initialize(
        IContractManager contractManagerOfSkaleManagerValue,
        ILinker linker,
        IMessageProxyForMainnet messageProxyValue

ILinker would be spesified as invalid cause of no Linker has been called from parent call.

Gas opt

#1 Using 1 modifier to check ROLE https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L116-L135 currently this contract is using 3 modifier to check which address is validated to run the function. Just use 1 modifier and pass which role will run the function then pass the address into the modifier argument:

modifier onlyRole(bytes32 _role) {
        require(hasRole(_role, msg.sender), "ROLE is required");
        _;
    }
//then run the function:
function foo(bytes) onlyRole(anyRole){}

#2 Using && spend more gashttps://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/DepositBox.sol#L77-L81 instead of using &&, using multiple require check can save execution gas fee:

require(
            schainHash != keccak256(abi.encodePacked("Mainnet")),
            "Receiver chain is incorrect"
        );
require(
            sender == schainLinks[schainHash],
            "Receiver chain is incorrect"
        );

#3 Using new private function to validate connectedChains[value].inited https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L240 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L258 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L291 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L315 create a new private function to run:

require(connectedChains[value].inited, "Error");

than call the code 4 times can save 18.018 gas when deploying the contract:

 function _foo(string memory _x)private view{
	require(connectedChains[_x].inited, "error");
}
 function removeConnectedChain(string memory schainName) public virtual override onlyChainConnector {
        bytes32 schainHash = keccak256(abi.encodePacked(schainName));
        _foo(schainName);
        delete connectedChains[schainHash];
    }    

#4 better increment for saving more gas

Prefix increment are cheaper than postfix increment, its a common that using ++i instead i++ for compiler ^0.8.* .So this f() are not using prefix increments (++i) or using the unchecked (++i).

it can be seen from here : https://github.com/code-423n4/2022-01-xdefi-findings/issues/9 and

Occurance :

blob/main/contracts/schain/TokenManagers/TokenManagerERC1155.sol 	#L500, #L514 
blob/main/contracts/schain/MessageProxyForSchain.sol  			#L118, #L222 
blob/main/contracts/schain/TokenManagerLinker.sol			#L151, #L166, #L192		
blob/main/contracts/MessageProxy.sol					#L221, #L515
blob/main/contracts/mainnet/Linker.sol					#L100, #L149, #L175
blob/main/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol		#L76, #L276
blob/main/contracts/mainnet/DepositBoxes/DepositBoxERC721.sol		#L76, #L260
blob/main/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol		#L79, #L275, #L398, #L444, #L459

#5 state variables that could be set as immutable

the erc721ContractOnSchain & receiverContractOnMainnet could be set as immutable to save more gas

Occurance :

blob/main/contracts/extensions/ERC721ReferenceMintAndMetadataSchain.sol #L34-35
blob/main/contracts/extensions/ERC721ReferenceMintAndMetadataMainnet.sol #L34

#6 Custom errors from Solidity 0.8.4 are cheaper than revert strings. https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L173 instead of using string to revert the error message, use error to declare the custom error, then replace the revert string with it.

error errorMessage();

#7 using ++var instead += 1 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L302 change l 302 to:

++connectedChains[targetChainHash].outgoingMessageCounter;

#8 Using storage can save gas https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/Messages.sol#L208 instead of caching message in memory, read it from storage can save gas. The message var is just called once inl L 213

 TransferEthMessage storage message = TransferEthMessage(
            BaseMessage(MessageType.TRANSFER_ETH),
            receiver,
            amount
        );

#9 unnecessary length declaration https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/Linker.sol#L173 length var is just used once in the L 175. Its unnecessary to chace it in memory. Remove the line and call _mainnetContracts.length() directly:

for (uint i = 0; connected && i <  _mainnetContracts.length(); i++)

#10 Dont set uint i = 0 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/MessageProxyForMainnet.sol#L118 by just declaring uint i; without set the value = 0 can save deployment gas fee. Do it on all for() loop

#11 Set the default value on vars declaration https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/MessageProxyForMainnet.sol#L73-L74 by setting the value of headerMessageGasCost and messageGasCost on their declaration, can save gas without writing storage in initialize(): https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/MessageProxyForMainnet.sol#L298-L299