SKALE contest - m_smirnova2020's results

1.Not needed check for uint > 0

Impact

The following functions check that an uint > 0 but it's always true.

Proof of Concept

https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/

DepositBoxEth.sol#L122

There are several other places throughout the codebase where the same optimization

can be used.

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

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the checks.

2.Prefix increaments are cheaper than postfix increaments

Impact

The functions use prefix increaments (i ++) instead of postfix increaments (++ i). Prefix increaments are cheaper than postfix increaments.

Proof of Concept

https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/

DepositBoxERC721.sol#L76 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/

DepositBoxERC721.sol#L260 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/

DepositBoxERC1155.sol#L79 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/

DepositBoxERC1155.sol#L275

There are several other places throughout the codebase where the same optimization

can be used.

for (uint256 i = 0; i < tokens.length; i++) {

Tools Used

Manual analysis

Recommended Mitigation Steps

Change all prefix increaments to postfix increaments where it doesn't affects the

functionality.

3.Long Revert Strings

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and

will decrease runtime gas when the revert condition has been met. Revert strings that are longer than 32 bytes require at least one additional

mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyF

orMainnet.sol#L152 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyF

orMainnet.sol#L171 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyF

orMainnet.sol#L186-L190 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyF

orMainnet.sol#L191 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyF

orMainnet.sol#L217 There are several other places throughout the codebase where the same optimization

can be used.

require(address(newCommunityPoolAddress) != address(0), "CommunityPool address has 

to be set");

Tools Used

Manual analysis

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes. Or consider moving to solc 0.8.4 or

greater and use Custom Errors.

4.Unnecessary initialization of loop index variable

Impact

For loop indices across the contract functions use explicit 0 initializations which

are not required because the default value of uints is 0. Removing this explicit

unnecessary initialization will save a little gas.

Proof of Concept

https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/Linker.sol#L1

00 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/Linker.sol#L1

49 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/Linker.sol#L1

75 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyF

orMainnet.sol#L118 https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyF

orMainnet.sol#L235 There are several other places throughout the codebase where the same optimization

can be used.

for (uint i = 0; i < schainContracts.length; i++) {

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove unnecessary initialization of loop index variable

5.Change string to byteX if possible

Impact

Declaring in the string public schainName` with type bytes32 can save gas.

Proof of Concept

https://github.com/skalenetwork/ima-c4-

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/extensions/ERC721Refe

renceMintAndMetadataMainnet.sol#L36

string public schainName;

Tools

Manual analysis

6.Use Custom Errors to save Gas

Impact

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Source: https://blog.soliditylang.org/2021/04/21/custom-errors/: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to

explain to users why an operation failed through the use of custom errors. Until

now, you could already use strings to give more information about failures (e.g.,

revert("Insufficient funds.");), but they are rather expensive, especially when it

comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and

outside of contracts (including interfaces and libraries). require(_locked == 0, "LOCKED");

Proof of Concept

https://github.com/skalenetwork/ima-c4-

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

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

audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L133 There are several other places throughout the codebase where the same optimization

can be used.

require(hasRole(CHAIN_CONNECTOR_ROLE, msg.sender), "CHAIN_CONNECTOR_ROLE is 

required");

Tools

Manual analysis

Recommended Mitigation Steps

Replace revert strings with custom errors.

7. Cache array length in for loops can save gas

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol#L265 function getFunds( string calldata schainName, address erc1155OnMainnet, address receiver, uint256[] memory ids, uint256[] memory amounts ) external override onlySchainOwner(schainName) whenKilled(keccak256(abi.encodePacked(schainName))) { bytes32 schainHash = keccak256(abi.encodePacked(schainName)); require(ids.length == amounts.length, "Incorrect length of arrays"); for (uint256 i = 0; i < ids.length; i++) {

Tools Used

Manual analysis

Recommended Mitigation Steps

Consider to cache array length.