Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gas Optimizations #34

Open
code423n4 opened this issue Mar 6, 2022 · 2 comments
Open

Gas Optimizations #34

code423n4 opened this issue Mar 6, 2022 · 2 comments
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Gas Report

Table of Contents:

Foreword

  • Storage-reading optimizations

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). In the paragraphs below, please see the @audit-issue tags in the pieces of code's comments for more information about SLOADs that could be saved by caching the mentioned storage variables in memory variables.

  • @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

File: CollateralizedDebt.sol

modifier onlyConvenience()

Inline a modifier that's only used once

As onlyConvenience() is only used once in this contract (in function mint()), it should get inlined to save gas:

File: CollateralizedDebt.sol
80:     modifier onlyConvenience() {
81:         require(msg.sender == address(convenience), 'E403');
82:         _;
83:     }
84: 
85:     function mint(address to, uint256 id) external override onlyConvenience { //@audit onlyConvenience modifier only used only here: inline it.
86:         _safeMint(to, id);
87:     }

File: TimeswapPair.sol

function mint()

Use memory variables for calculation

The code can be optimized from this:

File: TimeswapPair.sol
185:         pool.state.x += param.xIncrease;
186:         pool.state.y += param.yIncrease;
187:         pool.state.z += param.zIncrease;
...
193:         emit Sync(param.maturity, pool.state.x, pool.state.y, pool.state.z); //@audit can save 3 SLOADs by using memory for calc

to this:

File: TimeswapPair.sol
185:         (uint112 _poolStateX, uint112 _poolStateY, uint112 _poolStateZ) = (pool.state.x + param.xIncrease, pool.state.y + param.yIncrease, pool.state.z + param.zIncrease);
186: 
187:         pool.state.x = _poolStateX;
188:         pool.state.y = _poolStateY;
189:         pool.state.z = _poolStateZ;
...
195:         emit Sync(param.maturity, _poolStateX, _poolStateY, _poolStateZ);

function burn()

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

File: TimeswapPair.sol
225:         require(pool.state.totalLiquidity > 0, 'E206'); //@audit should be != 0

Also, please enable the Optimizer.

function lend()

Use memory variables for calculation

Just like in function mint() ( Use memory variables for calculation ), the code can be optimized here by caching the new values for pool.state.x, pool.state.y and pool.state.z :

File: TimeswapPair.sol
310:         pool.state.x += param.xIncrease;
311:         pool.state.y -= param.yDecrease;
312:         pool.state.z -= param.zDecrease;
...
320:         emit Sync(param.maturity, pool.state.x, pool.state.y, pool.state.z);//@audit can save 3 SLOADs by using memory for calc

The same way, the final code will look like this (with the difference that yDecrease and zDecreased are used here):

         (uint112 _poolStateX, uint112 _poolStateY, uint112 _poolStateZ) = (pool.state.x + param.xIncrease, pool.state.y - param.yDecrease, pool.state.z - param.zDecrease);
 
         pool.state.x = _poolStateX;
         pool.state.y = _poolStateY;
         pool.state.z = _poolStateZ;
...
         emit Sync(param.maturity, _poolStateX, _poolStateY, _poolStateZ);

function borrow()

Use memory variables for calculation

Just like in function mint() ( Use memory variables for calculation ) and function lend(), the code can be optimized here by caching the new values for pool.state.x, pool.state.y and pool.state.z :

File: TimeswapPair.sol
432:         pool.state.x -= param.xDecrease;
433:         pool.state.y += param.yIncrease;
434:         pool.state.z += param.zIncrease;
...
444:         emit Sync(param.maturity, pool.state.x, pool.state.y, pool.state.z);//@audit can save 3 SLOADs by using memory for calc

The same way, the final code will look like this (with the difference that xDecrease is used here):

         (uint112 _poolStateX, uint112 _poolStateY, uint112 _poolStateZ) = (pool.state.x - param.xDecrease, pool.state.y + param.yIncrease, pool.state.z + param.zIncrease);
 
         pool.state.x = _poolStateX;
         pool.state.y = _poolStateY;
         pool.state.z = _poolStateZ;
...
         emit Sync(param.maturity, _poolStateX, _poolStateY, _poolStateZ);

function pay()

An array's length should be cached to save gas in for-loops

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.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

File: TimeswapPair.sol
480:         for (uint256 i; i < param.ids.length;) { //@audit cache this

General recommendation

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

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).

Instances include:

Timeswap/Convenience/contracts/BondInterest.sol:61:        require(msg.sender == address(convenience), 'E403');
Timeswap/Convenience/contracts/BondPrincipal.sol:61:        require(msg.sender == address(convenience), 'E403');
Timeswap/Convenience/contracts/CollateralizedDebt.sol:45:        require(_owners[id] != address(0), 'E404');
Timeswap/Convenience/contracts/CollateralizedDebt.sol:62:        require(id < pair.totalDuesOf(maturity, address(convenience)), 'E614');
Timeswap/Convenience/contracts/CollateralizedDebt.sol:81:        require(msg.sender == address(convenience), 'E403');
Timeswap/Convenience/contracts/InsuranceInterest.sol:61:        require(msg.sender == address(convenience), 'E403');
Timeswap/Convenience/contracts/InsurancePrincipal.sol:61:        require(msg.sender == address(convenience), 'E403');
Timeswap/Convenience/contracts/Liquidity.sol:52:        require(msg.sender == address(convenience), 'E403');
Timeswap/Convenience/contracts/TimeswapConvenience.sol:64:        require(address(_factory) != address(0), 'E601');
Timeswap/Convenience/contracts/TimeswapConvenience.sol:65:        require(address(_weth) != address(0), 'E601');
Timeswap/Convenience/contracts/TimeswapConvenience.sol:66:        require(address(_factory) != address(_weth), 'E612');
Timeswap/Convenience/contracts/TimeswapConvenience.sol:75:        require(msg.sender == address(weth));
Timeswap/Convenience/contracts/TimeswapConvenience.sol:560:        require(msg.sender == address(pair), 'E701');
Timeswap/Convenience/contracts/TimeswapConvenience.sol:584:        require(msg.sender == address(pair), 'E701');
Timeswap/Convenience/contracts/TimeswapConvenience.sol:598:        require(msg.sender == address(pair), 'E701');
Timeswap/Convenience/contracts/TimeswapConvenience.sol:612:        require(msg.sender == address(pair), 'E701');
Timeswap/Core/contracts/TimeswapFactory.sol:38:        require(_owner != address(0), 'E101');
Timeswap/Core/contracts/TimeswapFactory.sol:48:        require(asset != collateral, 'E103');
Timeswap/Core/contracts/TimeswapFactory.sol:49:        require(asset != IERC20(address(0)) && collateral != IERC20(address(0)), 'E101');
Timeswap/Core/contracts/TimeswapFactory.sol:50:        require(getPair[asset][collateral] == IPair(address(0)), 'E104');
Timeswap/Core/contracts/TimeswapFactory.sol:61:        require(msg.sender == owner, 'E102');
Timeswap/Core/contracts/TimeswapFactory.sol:62:        require(_pendingOwner != address(0), 'E101');
Timeswap/Core/contracts/TimeswapFactory.sol:70:        require(msg.sender == pendingOwner, 'E102');
Timeswap/Core/contracts/TimeswapPair.sol:131:        require(locked == 1, 'E211');
Timeswap/Core/contracts/TimeswapPair.sol:151:        require(block.timestamp < param.maturity, 'E202');
Timeswap/Core/contracts/TimeswapPair.sol:152:        unchecked { require(param.maturity - block.timestamp < 0x100000000, 'E208'); }
Timeswap/Core/contracts/TimeswapPair.sol:153:        require(param.liquidityTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:154:        require(param.dueTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:155:        require(param.liquidityTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:156:        require(param.dueTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:157:        require(param.xIncrease != 0, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:158:        require(param.yIncrease != 0, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:159:        require(param.zIncrease != 0, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:172:        require(liquidityOut != 0, 'E212');
Timeswap/Core/contracts/TimeswapPair.sol:217:        require(block.timestamp >= param.maturity, 'E203');
Timeswap/Core/contracts/TimeswapPair.sol:218:        require(param.assetTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:219:        require(param.collateralTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:220:        require(param.assetTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:221:        require(param.collateralTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:222:        require(param.liquidityIn != 0, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:225:        require(pool.state.totalLiquidity > 0, 'E206');
Timeswap/Core/contracts/TimeswapPair.sol:273:        require(block.timestamp < param.maturity, 'E202');
Timeswap/Core/contracts/TimeswapPair.sol:274:        require(param.bondTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:275:        require(param.insuranceTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:276:        require(param.bondTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:277:        require(param.insuranceTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:278:        require(param.xIncrease != 0, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:281:        require(pool.state.totalLiquidity != 0, 'E206');
Timeswap/Core/contracts/TimeswapPair.sol:342:        require(block.timestamp >= param.maturity, 'E203');
Timeswap/Core/contracts/TimeswapPair.sol:343:        require(param.assetTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:344:        require(param.collateralTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:345:        require(param.assetTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:346:        require(param.collateralTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:347:        require(
Timeswap/Core/contracts/TimeswapPair.sol:401:        require(block.timestamp < param.maturity, 'E202');
Timeswap/Core/contracts/TimeswapPair.sol:402:        require(param.assetTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:403:        require(param.dueTo != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:404:        require(param.assetTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:405:        require(param.dueTo != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:406:        require(param.xDecrease != 0, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:409:        require(pool.state.totalLiquidity != 0, 'E206');
Timeswap/Core/contracts/TimeswapPair.sol:468:        require(block.timestamp < param.maturity, 'E202');
Timeswap/Core/contracts/TimeswapPair.sol:469:        require(param.owner != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:470:        require(param.to != address(0), 'E201');
Timeswap/Core/contracts/TimeswapPair.sol:471:        require(param.to != address(this), 'E204');
Timeswap/Core/contracts/TimeswapPair.sol:472:        require(param.ids.length == param.assetsIn.length, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:473:        require(param.ids.length == param.collateralsOut.length, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:478:        require(dues.length >= param.ids.length, 'E205');
Timeswap/Core/contracts/TimeswapPair.sol:482:            require(due.startBlock != BlockNumber.get(), 'E207');
Timeswap/Core/contracts/TimeswapPair.sol:483:            if (param.owner != msg.sender) require(param.collateralsOut[i] == 0, 'E213');
Timeswap/Core/contracts/TimeswapPair.sol:484:            require(uint256(assetIn) * due.collateral >= uint256(collateralOut) * due.debt, 'E303');
Timeswap/Core/contracts/TimeswapPair.sol:514:        require(msg.sender == factory.owner(), 'E216');

I suggest replacing revert strings with custom errors.
@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Mar 6, 2022
code423n4 added a commit that referenced this issue Mar 6, 2022
@code423n4
Dravee issue #34
8748165
@amateur-dev amateur-dev added sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") and removed sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels Mar 8, 2022
@Mathepreneur
Copy link
Collaborator

Timeswap-Labs/Timeswap-V1-Convenience@7434c3d
Timeswap-Labs/Timeswap-V1-Core@7055734

@Mathepreneur Mathepreneur added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label Mar 9, 2022
@Mathepreneur
Copy link
Collaborator

Some implementation can't be implemented due to stack too deep error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@amateur-dev @Mathepreneur @code423n4