code423n4 commented 2 years ago

Gas Reduction opportunities

To check the actual size of the reduction, hardhat-gas-reporter is used. ( https://www.npmjs.com/package/hardhat-gas-reporter ). At each result, it lists how many size of the gas is reduced after the change.

`mainnet/CommunityPool.sol`

Title: `refundGasByUser` function can use unchecked directory and slight refactor may reduce gas fee.

By refactoring following code, it can reduce gas cost of CommunityPool.sol. https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/CommunityPool.sol#L97-L111

uint amount = tx.gasprice * gas;
if (amount > _userWallets[user][schainHash]) {
    amount = _userWallets[user][schainHash];
}
_userWallets[user][schainHash] = _userWallets[user][schainHash] - amount;
if (!_balanceIsSufficient(schainHash, user, 0)) {
    activeUsers[user][schainHash] = false;
    messageProxy.postOutgoingMessage(
        schainHash,
        schainLinks[schainHash],
        Messages.encodeLockUserMessage(user)
    );
}
node.sendValue(amount);
return (tx.gasprice * gas - amount) / tx.gasprice;

First, _userWallets[user][schainHash] - amount will never be less than 0 so can be wrapped by unchecked, since amount is always equal to or less than _userWallets[user][schainHash]. Second, (tx.gasprice * gas - amount) / tx.gasprice will not underflown so can be wrapped by unchecked directory, since tx.gasprice * gas - amount will be equal to or more than 0.

Here is an example of the modified code:

uint amount = tx.gasprice * gas;
if (amount > _userWallets[user][schainHash]) {
    amount = _userWallets[user][schainHash];
}
unchecked {
    _userWallets[user][schainHash] = _userWallets[user][schainHash] - amount;
}
if (!_balanceIsSufficient(schainHash, user, 0)) {
    activeUsers[user][schainHash] = false;
    messageProxy.postOutgoingMessage(
        schainHash,
        schainLinks[schainHash],
        Messages.encodeLockUserMessage(user)
    );
}
node.sendValue(amount);
unchecked {
    return (tx.gasprice * gas - amount) / tx.gasprice;
}

It simply wraps the above mentioned code with unchecked directory.

Here is the comparison of the gas cost at CommunityPool.sol

Before: 2114911
After: 2100269
Before - After: 14642 (About 0.6% reduction)

Title: `withdrawFunds` function can reduce the gas cost by reordering the condition of if statement.

In the if statement, it calls _balanceIsSufficient function first. But it can check activeUsers[msg.sender][schainHash] to reduce the gas cost slightly. https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/CommunityPool.sol#L174-L184

if (
    !_balanceIsSufficient(schainHash, msg.sender, 0) &&
    activeUsers[msg.sender][schainHash]
) {
    activeUsers[msg.sender][schainHash] = false;
    messageProxy.postOutgoingMessage(
        schainHash,
        schainLinks[schainHash],
        Messages.encodeLockUserMessage(msg.sender)
    );
}

Here is an example of the modification.

if (
    activeUsers[msg.sender][schainHash] &&
    !_balanceIsSufficient(schainHash, msg.sender, 0)
) {
    activeUsers[msg.sender][schainHash] = false;
    messageProxy.postOutgoingMessage(
        schainHash,
        schainLinks[schainHash],
        Messages.encodeLockUserMessage(msg.sender)
    );
}

Here is the comparison of the gas cost at CommunityPool.sol

Before: 2114911
After: 2113003
Before - After: 1908 (About 0.09% reduction)

Title: `withdrawFunds` function can reduce gas cost by using unchecked

_userWallets[msg.sender][schainHash] - amount will never be less than 0, since it checks amount <= _userWallets[msg.sender][schainHash] at require function.

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

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

Here is an example of the modification.

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

Here is the comparison of the gas cost at CommunityPool.sol

Before: 2114911
After: 2107176
Before - After: 7735 (About 0.36% reduction)

`mainnet/MessageProxy.sol`

Title: `i++` can be wrapped by `unchecked` directory.

mainnet/MessageProxy.sol and mainnet/MessageProxyForMainnet.sol contains for loop, but i++ can be wrapped by unchecked directory to decrease the gas cost.

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L221 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L515 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/MessageProxyForMainnet.sol#L118 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/MessageProxyForMainnet.sol#L235

Here is an example of the modification.

for (uint256 i = 0; i < messages.length; ) {
    data = abi.encodePacked(
        data,
        bytes32(bytes20(messages[i].sender)),
        bytes32(bytes20(messages[i].destinationContract)),
        messages[i].data
    );
    unchecked {
        i++;
    }
}

Here is the comparison of the gas cost at MessageProxyForMainnet.sol

Before: 3403300
After: 3388788
Before - After: 14512 (About 0.4% reduction)

In addition to MessageProxy.sol, here are other opportunities to decrease gas cost by wrapping i++ by unchecked directory at following files:

yavrsky commented 2 years ago

We prefer not to use "unchecked" for security reasons, even if it is applicable there. Also only marginal gas improvements.

GalloDaSballo commented 2 years ago

To add #47 which is valued at 62500

GalloDaSballo commented 2 years ago

Title: refundGasByUser function can use unchecked directory and slight refactor may reduce gas fee.

Unchecked should save 20 gas per operation * 4 = 80 gas

Title: withdrawFunds function can reduce the gas cost by reordering the condition of if statement.

Valid but ultimately saves gas on the "bad path"

Title: withdrawFunds function can reduce gas cost by using unchecked

Saves 20 gas

Title: i++ can be wrapped by unchecked directory.

Saves 20 gas per iteration * 4 = 80

11 * 20 = 220

Additional gas saved: 400

Total Combined: 62900

code-423n4 / 2022-02-skale-findings

Gas Optimizations #46

Gas Reduction opportunities

`mainnet/CommunityPool.sol`

Title: `refundGasByUser` function can use unchecked directory and slight refactor may reduce gas fee.

Title: `withdrawFunds` function can reduce the gas cost by reordering the condition of if statement.

Title: `withdrawFunds` function can reduce gas cost by using unchecked

`mainnet/MessageProxy.sol`

Title: `i++` can be wrapped by `unchecked` directory.

Title: refundGasByUser function can use unchecked directory and slight refactor may reduce gas fee.

Title: withdrawFunds function can reduce the gas cost by reordering the condition of if statement.

Title: withdrawFunds function can reduce gas cost by using unchecked

Title: i++ can be wrapped by unchecked directory.