code423n4 commented 2 years ago

Title

Massive gas reduction just by reusing common logic in library

Vulnerability details

Impact

Massive gas cost and contract size reductions. Since the total gas cost reduction is 518297 and the total size reduction is 2.397 KB, this report is shared as an individual report.

Proof of Concept

The function keccak256(abi.encodePacked(...)) is called at many places in the codebase.

Here are lists of files which uses keccak256(abi.encodePacked(...)) function several times in mainnet directory. By using the function in the library instead of calling `keccak256(abi.encodePacked(...))`` function reach time can reduce the gas cost hugely.

/contracts/MessageProxy.sol
/contracts/mainnet/CommunityPool.sol
/contracts/mainnet/DepositBox.sol
/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol b/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol
/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol b/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol
/contracts/mainnet/DepositBoxes/DepositBoxERC721.sol b/contracts/mainnet/DepositBoxes/DepositBoxERC721.sol
/contracts/mainnet/DepositBoxes/DepositBoxEth.sol b/contracts/mainnet/DepositBoxes/DepositBoxEth.sol
/contracts/mainnet/HashLibrary.sol b/contracts/mainnet/HashLibrary.sol
/contracts/mainnet/MessageProxyForMainnet.sol b/contracts/mainnet/MessageProxyForMainnet.sol
/contracts/mainnet/Twin.sol b/contracts/mainnet/Twin.sol

Linker.sol and SkaleManagerClient.sol also use keccak256(abi.encodePacked(...)) function, but they use only this function once, and in this case, using library increases the gas cost. So in this report, Linker.sol and SkaleManagerClient.sol keep using keccak256(abi.encodePacked(...)) function.

Tools Used

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.

Recommended Mitigation Steps

First, create library which contains function to convert string calldata schainName and string memory schainName into bytes32 using keccak256(abi.encodePacked(...)) function.

library HashLibrary {
    function getHashFromCalldata(string calldata schainName) internal pure returns(bytes32) {
        return keccak256(abi.encodePacked(schainName));
    }    

    function getHashFromMemory(string memory schainName) internal pure returns(bytes32) {
        return keccak256(abi.encodePacked(schainName));
    }    
}

Then use these functions in HashLibrary library in each codebase. Here is an example of the modification.

// After
function rechargeUserWallet(string calldata schainName, address user) external payable override {
    bytes32 schainHash = HashLibrary.getHashFromCalldata(schainName);

// Before
function rechargeUserWallet(string calldata schainName, address user) external payable override {
    bytes32 schainHash = keccak256(abi.encodePacked(schainName));

Here are results of the gas reduction observed at the local environment.

Deployments	Before-Avg (Runs:200)	After-Avg (Runs:200)	Improvement(%)
CommunityPool	2114911	2083814	1.47%
DepositBoxERC1155	4157660	4023142	3.23%
DepositBoxERC20	3207840	3102364	3.29%
DepositBoxERC721	3036407	2930860	3.47%
DepositBoxEth	2625776	2541459	3.21%
Linker	1805737	1794702	0.61%
MessageProxyForMainnet	3403300	3356993	1.36%

Here are results of the contract size reduction at the local environment.

Contract Name	Size (KB)	Change (KB)
Linker	8.069	0.051
CommunityPool	9.409	0.144
DepositBoxEth	11.528	0.390
DepositBoxERC721	13.330	0.488
DepositBoxERC20	14.124	0.488
MessageProxyForMainnet	15.300	0.214
DepositBoxERC1155	18.389	0.622

In total, the total contract size reduction is 2.397 KB after the change.

Here is an output of git diff that can reduce the above mentioned gas usage.

https://gist.github.com/TerrierLover/9c80fe4099ea149b7e2fc559fa8a844e

The base commit of the above git diff output is https://github.com/skalenetwork/ima-c4-audit/commit/11d6a6ae5bf16af552edd75183791375e501915f .

yavrsky commented 2 years ago

Gas optimization for contract deployments is not our prior and 500K gas cost reduction is not that big relating to deployment all contracts, but good catch though.

GalloDaSballo commented 2 years ago

Ultimately this will save a ton of deployment gas, for the sake of judging I'll rate it as if it saved 25 * 2500 (similar to the Messages Finding) 62500

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