Gas Optimizations

[code423n4]

Gas Optimizations

Save the return value of a function instead of calling it multiple times

Let's take a look at the getContractRegisteredRange function in the MessageProxy contract:

function getContractRegisteredRange(
    bytes32 schainHash,
    uint256 from,
    uint256 to
)
    external
    view
    override
    returns (address[] memory contractsInRange)
{
    require(
        from < to && to - from <= 10 && to <= _getRegistryContracts()[schainHash].length(),
        "Range is incorrect"
    );
    contractsInRange = new address[](to - from);
    for (uint256 i = from; i < to; i++) {
        contractsInRange[i - from] = _getRegistryContracts()[schainHash].at(i);
    }
}

In this function, the expression _getRegistryContracts()[schainHash] is used and calculated multiple times, and it can be calculated once. The code will look something like this:

function getContractRegisteredRange(
    bytes32 schainHash,
    uint256 from,
    uint256 to
)
    external
    view
    override
    returns (address[] memory contractsInRange)
{
    EnumerableSetUpgradeable.AddressSet storage set = _getRegistryContracts()[schainHash];
    require(
        from < to && to - from <= 10 && to <= set.length(),
        "Range is incorrect"
    );
    contractsInRange = new address[](to - from);
    for (uint256 i = from; i < to; i++) {
        contractsInRange[i - from] = set.at(i);
    }
}

This can be done also in the registerExtraContractForAll function of the MessageProxy contract:

function registerExtraContractForAll(address extraContract) external override onlyExtraContractRegistrar {
    require(extraContract.isContract(), "Given address is not a contract");
    require(!_getRegistryContracts()[bytes32(0)].contains(extraContract), "Extra contract is already registered");
    _getRegistryContracts()[bytes32(0)].add(extraContract);
    emit ExtraContractRegistered(bytes32(0), extraContract);
}

We can save the return value of _getRegistryContracts()[bytes32(0)] instead of calling it twice. The code will be:

function registerExtraContractForAll(address extraContract) external override onlyExtraContractRegistrar {
    require(extraContract.isContract(), "Given address is not a contract");
    EnumerableSetUpgradeable.AddressSet storage set = _getRegistryContracts()[bytes32(0)];
    require(!set.contains(extraContract), "Extra contract is already registered");
    set.add(extraContract);
    emit ExtraContractRegistered(bytes32(0), extraContract);
}

loop in getContractRegisteredRange (also saving a variable instead of i - from)

Loops can be optimized in several ways. Let's look for example at the loop in the getContractRegisteredRange function of the MessageProxy contract:

function getContractRegisteredRange(
    bytes32 schainHash,
    uint256 from,
    uint256 to
)
    external
    view
    override
    returns (address[] memory contractsInRange)
{
    require(
        from < to && to - from <= 10 && to <= _getRegistryContracts()[schainHash].length(),
        "Range is incorrect"
    );
    contractsInRange = new address[](to - from);
    for (uint256 i = from; i < to; i++) {
        contractsInRange[i - from] = _getRegistryContracts()[schainHash].at(i);
    }
}

We can do several things here:

1. Use ++i instead of i++ to save gas (this is a cheaper operation that will do the same in this case)
2. Prevent calculating i - from in every iteration by saving another index. The code will look something like this:

   function getContractRegisteredRange(
   bytes32 schainHash,
   uint256 from,
   uint256 to
   )
   external
   view
   override
   returns (address[] memory contractsInRange)
   {
   require(
       from < to && to - from <= 10 && to <= _getRegistryContracts()[schainHash].length(),
       "Range is incorrect"
   );
   contractsInRange = new address[](to - from);
   uint index;
   for (uint256 i = from; i < to; ++i) {
       contractsInRange[index++] = _getRegistryContracts()[schainHash].at(i);
   }
   }

calculating an expression once

Another thing that can be done in this specific function (getContractRegisteredRange function of the MessageProxy contract) is that the expression to - from can be calculated once instead of twice, and can be calculated using unchecked becuase of the check above. The code will look like this:

// ...
require(from < to, "Range is incorrect");
unchecked {
    uint len = to - from;
    require(len <= 10, "Range is incorrect");
    contractsInRange = new address[](len);
}
// ...

no need to call contains in most cases

In a lot of places in the contracts you call the contains function to check if an element in the set, and only then removing or adding it. But you can just check the return value of the add or remove function - add returns true if the element was added successfully, which means that he wasn't in the set before. The remove function returns true if the element was removed successfully, which means that he wasn in the set before. So instead of writing something like this:

require(!_getRegistryContracts()[bytes32(0)].contains(extraContract), "Extra contract is already registered");
_getRegistryContracts()[bytes32(0)].add(extraContract);

You can simply use:

require(_getRegistryContracts()[bytes32(0)].add(extraContract), "Extra contract is already registered");

This will save the gas spent on the call to the contains function.

This repeats all over the contracts, the places I saw this happening

• MessageProxy contract - registerExtraContractForAll, removeExtraContractForAll, _registerExtraContract, _removeExtraContract functions
• In the DepositBox and TokenManager contracts There might be more places that this repeats in, but it's worth changing - this is a redundant function call.

loops in MessageProxyForMainnet contract

In the MessageProxyForMainnet contrcat, there are some loops that can be optimize. Let's take a look at the loop in the initializeAllRegisteredContracts function:

for (uint256 i = 0; i < contracts.length; i++) {
    if (
        deprecatedRegistryContracts[schainHash][contracts[i]] &&
        !_registryContracts[schainHash].contains(contracts[i])
    ) {
        _registryContracts[schainHash].add(contracts[i]);
        delete deprecatedRegistryContracts[schainHash][contracts[i]];
    }
}

We can do several things here:

1. Remove the calls to contains like I explained before
2. Use ++i instead of i++, which is a cheaper operation (in this case there is no difference between i++ and ++i because we dont use the return value of this expression, which is the only difference between these two expression).
3. Save the contracts array length in a local variable instead of accessing it in every iteration.
4. Save contracts[i] in a local variable instead of accessing it 4 times in every iteration. This will save accssing the array's ith element 4 times in every iteration ,which requires an address calculation.
5. There's no need to initialize i to its default value, it will be done automatically and it will consume more gas if it will be done (I know, sounds stupid, but trust me - it works). So after applying all these changes, the loop will look something like this:

   address contracts_i;
   uint len = contracts.length;
   for (uint256 i; i < len; ++i) {
   contracts_i = contracts[i];
   if (
       deprecatedRegistryContracts[schainHash][contracts_i] &&
       _registryContracts[schainHash].add(contracts_i)
   ) {
       delete deprecatedRegistryContracts[schainHash][contracts_i];
   }
   }

   Another loop that can be optimized is in the postIncomingMessages of this contract:

   for (uint256 i = 0; i < messages.length; i++) {
   gasTotal = gasleft();
   if (isContractRegistered(bytes32(0), messages[i].destinationContract)) {
       address receiver = _getGasPayer(fromSchainHash, messages[i], startingCounter + i);
       _callReceiverContract(fromSchainHash, messages[i], startingCounter + i);
       notReimbursedGas += communityPool.refundGasByUser(
           fromSchainHash,
           payable(msg.sender),
           receiver,
           gasTotal - gasleft() + additionalGasPerMessage
       );
   } else {
       _callReceiverContract(fromSchainHash, messages[i], startingCounter + i);
       notReimbursedGas += gasTotal - gasleft() + additionalGasPerMessage;
   }
   }

   The things we can do here:

6. Use ++i instead of i++.
7. Save the messages array length in a local variable instead of accessing it in every iteration.
8. Save messages[i] in a local variable instead of accessing it multiple times in every iteration.
9. Not initializing i to its default value.
10. Increment startingCounter instead of calculating startingCounter + i in every iteration.

Optimizing the reentrency gaurd

modifier messageInProgressLocker() {
    require(!messageInProgress, "Message is in progress");
    messageInProgress = true;
    _;
    messageInProgress = false;
}

The reenterncy guard in the MessageProxyForMainnet contract can be optimized. In solidity, changing a variable value from its default value to another value costs more gas then changing it from a non-default value to a non-default value (really weird, I know). So in the current implementation the operation of changing the lock from false to true can be optimized. In order to optimize it, you can use uint8 instead of bool, and use values 1 and 2 for the lock (1 for locked, 2 for unlocked). This will look like this:

uint8 messageInProgress = 1;
modifier messageInProgressLocker() {
    require(messageInProgress != 2, "Message is in progress");
    messageInProgress = 2;
    _;
    messageInProgress = 1;
}

Loops in the DepositBox contracts

There are lots of loops in these contrcats, and the tricks I explained before will work here too. Every optimization will save gas - even changing the i++ to ++i.

save the element and hash calculation instead of calculating it in every iteration in getSchainToAllERC721

In the mentioned function, the hash is calculated in order to access a mapping element. But instead of calculating it once and save the hash value, it's being calculated in every iteration, and that will cost extra gas for nothing. More than that, we can save all set instead of accessing the mapping in every interation.

Code before:

function getSchainToAllERC721(
    string calldata schainName,
    uint256 from,
    uint256 to
)
    external
    view
    override
    returns (address[] memory tokensInRange)
{
    require(
        from < to && to - from <= 10 && to <= _schainToERC721[keccak256(abi.encodePacked(schainName))].length(),
        "Range is incorrect"
    );
    tokensInRange = new address[](to - from);
    for (uint256 i = from; i < to; i++) {
        tokensInRange[i - from] = _schainToERC721[keccak256(abi.encodePacked(schainName))].at(i);
    }
}

Code after:

function getSchainToAllERC721(
    string calldata schainName,
    uint256 from,
    uint256 to
)
    external
    view
    override
    returns (address[] memory tokensInRange)
{
    EnumerableSetUpgradeable.AddressSet set = _schainToERC721[keccak256(abi.encodePacked(schainName))];
    require(
        from < to && to - from <= 10 && to <= set.length(),
        "Range is incorrect"
    );
    tokensInRange = new address[](to - from);
    for (uint256 i = from; i < to; i++) {
        tokensInRange[i - from] = set.at(i);
    }
}

[yavrsky]

Only marginal gas improvements.

[GalloDaSballo]

Save the return value of a function instead of calling it multiple times

I'm not convinced that storing the storage pointer will actually save gas

loop in getContractRegisteredRange (also saving a variable instead of i - from)

I believe this would save 6 gas per iteraton

no need to call contains in most cases

Will save 100 gas per instance -> 5 = 500

More loop savings

6 gas

Optimizing the reentrancy guard

This is a pretty meaningful gas saving in that the current one will cost 20k to set and refund 15k (gas refunds being capped at 1/5 call cost), while the proposed version will cost 5k to set and 5k to unset, with a refund because the original value is set back, this should bring the cost down meaningfully, saving 15k gas

Rest

15 gas

Total gas saved: 15527