Gas Optimizations #135

code423n4 · 2022-02-23T23:51:55Z

Gas Report

Table of Contents:

Gas Report

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.

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

@audit tags

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

Summary

One pattern that was often seen is caching structs in memory when it's not needed. A copy in memory of a storage struct will trigger as many SLOADs as there are slots. If the struct's fields are only read once, or if the number of storage reading would be inferior to the number of slots: don't cache the struct in memory.

File: AMM.sol

function initialize()

093:     function initialize(
094:         address _registry,
095:         address _underlyingAsset,
096:         string memory _name,//@audit readonly: calldata
097:         address _vamm,
098:         address _governance
099:     ) external initializer {
100:         _setGovernace(_governance);
101: 
102:         vamm = IVAMM(_vamm);
103:         underlyingAsset = _underlyingAsset;
104:         name = _name;
105:         fundingBufferPeriod = 15 minutes;
106: 
107:         syncDeps(_registry);
108:     }

Use `calldata` instead of `memory` for `string _name`

An external function passing a readonly variable should mark it as calldata and not memory

function openPosition()

113:     function openPosition(address trader, int256 baseAssetQuantity, uint quoteAssetLimit)
114:         override
115:         external
116:         onlyClearingHouse
117:         returns (int realizedPnl, uint quoteAsset, bool isPositionIncreased)
118:     {
119:         require(ammState == AMMState.Active, "AMM.openPosition.not_active");
120:         Position memory position = positions[trader]; //@audit 3 SLOADs vs 1 enough
121:         bool isNewPosition = position.size == 0 ? true : false;
122:         Side side = baseAssetQuantity > 0 ? Side.LONG : Side.SHORT;
123:         if (isNewPosition || (position.size > 0 ? Side.LONG : Side.SHORT) == side) {
124:             // realizedPnl = 0;
125:             quoteAsset = _increasePosition(trader, baseAssetQuantity, quoteAssetLimit);
126:             isPositionIncreased = true;
127:         } else {
128:             (realizedPnl, quoteAsset, isPositionIncreased) = _openReversePosition(trader, baseAssetQuantity, quoteAssetLimit);
129:         }
130:         _emitPositionChanged(trader, realizedPnl);
131:     }

Do not cache `positions[trader]` in memory

As a copy in memory of a struct makes as many SLOADs as there are slots, here a copy costs 3 SLOADs:

41:     struct Position {
42:         int256 size;
43:         uint256 openNotional;
44:         int256 lastPremiumFraction;
45:     }

However, only the size field is read twice. Therefore, only this field should get cached: int256 _size = positions[trader].size;

function liquidatePosition()

133:     function liquidatePosition(address trader)
134:         override
135:         external
136:         onlyClearingHouse
137:         returns (int realizedPnl, uint quoteAsset)
138:     {
139:         // don't need an ammState check because there should be no active positions
140:         Position memory position = positions[trader]; //@audit 3 SLOADs vs 1 enough
141:         bool isLongPosition = position.size > 0 ? true : false;
142:         // sending market orders can fk the trader. @todo put some safe guards around price of liquidations
143:         if (isLongPosition) {
144:             (realizedPnl, quoteAsset) = _reducePosition(trader, -position.size, 0);
145:         } else {
146:             (realizedPnl, quoteAsset) = _reducePosition(trader, -position.size, type(uint).max);
147:         }
148:         _emitPositionChanged(trader, realizedPnl);
149:     }

Do not cache `positions[trader]` in memory

Similar to Do not cache positions[trader] in memory.

However, only the size field is read 3 times. Therefore, only this field should get cached: int256 _size = positions[trader].size;

function removeLiquidity()

133:     function liquidatePosition(address trader)
134:         override
135:         external
136:         onlyClearingHouse
137:         returns (int realizedPnl, uint quoteAsset)
138:     {
139:         // don't need an ammState check because there should be no active positions
140:         Position memory position = positions[trader]; //@audit 3 SLOADs vs 1 enough
141:         bool isLongPosition = position.size > 0 ? true : false;
142:         // sending market orders can fk the trader. @todo put some safe guards around price of liquidations
143:         if (isLongPosition) {
144:             (realizedPnl, quoteAsset) = _reducePosition(trader, -position.size, 0);
145:         } else {
146:             (realizedPnl, quoteAsset) = _reducePosition(trader, -position.size, type(uint).max);
147:         }
148:         _emitPositionChanged(trader, realizedPnl);
149:     }

Do not cache `positions[maker]` in memory

Similar to Do not cache positions[trader] in memory.
However, here, even the fields shouldn't get cached, as they are read only once:

220:         Position memory _taker = positions[maker];//@audit 3 SLOADs vs 2 enough
...
233:             _taker.size,
234:             _taker.openNotional

Therefore, use 220: Position storage _taker = positions[maker];

Do not cache `makers[maker]` in memory

Similarly, a copy in memory for Maker costs 7 SLOADs:

48:     struct Maker {
49:         uint vUSD;
50:         uint vAsset;
51:         uint dToken;
52:         int pos; // position
53:         int posAccumulator; // value of global.posAccumulator until which pos has been updated
54:         int lastPremiumFraction;
55:         int lastPremiumPerDtoken;
56:     }

Here, caching the first 5 fields in memory is enough.

function getNotionalPositionAndUnrealizedPnl()

395:     function getNotionalPositionAndUnrealizedPnl(address trader)
396:         override
397:         external
398:         view
399:         returns(uint256 notionalPosition, int256 unrealizedPnl, int256 size, uint256 openNotional)
400:     {
401:         Position memory _taker = positions[trader];//@audit 3 SLOADs vs 2 enough
402:         Maker memory _maker = makers[trader];//@audit 7 SLOADs vs 3 enough
403: 
404:         (notionalPosition, size, unrealizedPnl, openNotional) = vamm.get_notional(
405:             _maker.dToken,
406:             _maker.vUSD,
407:             _maker.vAsset,
408:             _taker.size,
409:             _taker.openNotional
410:         );
411:     }

Do not cache `positions[trader]` in memory

Here, we need Position storage _taker = positions[trader];

Do not cache `makers[trader]` in memory

Here, we need Maker storage _maker = makers[trader];

function getPendingFundingPayment()

425:         Position memory taker = positions[trader];//@audit 3 SLOADs vs 2 enough
...
434:         Maker memory maker = makers[trader];//@audit 7 SLOADs vs 5 enough

Do not cache `positions[trader]` in memory

Here, we need Position storage _taker = positions[trader];

Do not cache `makers[trader]` in memory

Here, we need Maker storage _maker = makers[trader];

function getTakerNotionalPositionAndUnrealizedPnl()

458:     function getTakerNotionalPositionAndUnrealizedPnl(address trader) override public view returns(uint takerNotionalPosition, int256 unrealizedPnl) {
459:         Position memory position = positions[trader];//@audit 3 SLOADs vs 2 enough
460:         if (position.size > 0) {
461:             takerNotionalPosition = vamm.get_dy(1, 0, position.size.toUint256());
462:             unrealizedPnl = takerNotionalPosition.toInt256() - position.openNotional.toInt256();
463:         } else if (position.size < 0) {
464:             takerNotionalPosition = vamm.get_dx(0, 1, (-position.size).toUint256());
465:             unrealizedPnl = position.openNotional.toInt256() - takerNotionalPosition.toInt256();
466:         }
467:     }

Do not cache `positions[trader]` in memory

Here, we need to cache these fields: size and openNotional

function _emitPositionChanged()

527:     function _emitPositionChanged(address trader, int256 realizedPnl) internal {
528:         Position memory position = positions[trader];//@audit 3 SLOADs vs 2 enough
529:         emit PositionChanged(trader, position.size, position.openNotional, realizedPnl);
530:     }

Do not cache `positions[trader]` in memory

Here, we need Position storage _taker = positions[trader];

function _openReversePosition()

584:     function _openReversePosition(address trader, int256 baseAssetQuantity, uint quoteAssetLimit)
585:         internal
586:         returns (int realizedPnl, uint quoteAsset, bool isPositionIncreased)
587:     {
588:         Position memory position = positions[trader];//@audit 3 SLOADs vs 1 enough
589:         if (abs(position.size) >= abs(baseAssetQuantity)) {
590:             (realizedPnl, quoteAsset) = _reducePosition(trader, baseAssetQuantity, quoteAssetLimit);
591:         } else {
592:             uint closedRatio = (quoteAssetLimit * abs(position.size).toUint256()) / abs(baseAssetQuantity).toUint256();
593:             (realizedPnl, quoteAsset) = _reducePosition(trader, -position.size, closedRatio);
594: 
595:             // this is required because the user might pass a very less value (slippage-prone) while shorting
596:             if (quoteAssetLimit >= quoteAsset) {
597:                 quoteAssetLimit -= quoteAsset; //@audit uncheck (see L596)
598:             }
599:             quoteAsset += _increasePosition(trader, baseAssetQuantity + position.size, quoteAssetLimit);
600:             isPositionIncreased = true;
601:         }
602:     }

Do not cache `positions[trader]` in memory

Here, we need to cache the size field

Unchecked block L597

This line can't underflow due to the condition L596. Therefore, it should be wrapped in an unchecked block

function _calcTwap()

656:     function _calcTwap(uint256 _intervalInSeconds)
657:         internal
658:         view
659:         returns (uint256)
660:     {
661:         uint256 snapshotIndex = reserveSnapshots.length - 1; //@audit reserveSnapshots.length  SLOAD 1
662:         uint256 currentPrice = reserveSnapshots[snapshotIndex].lastPrice;
663:         if (_intervalInSeconds == 0) {
664:             return currentPrice;
665:         }
666: 
667:         uint256 baseTimestamp = _blockTimestamp() - _intervalInSeconds;
668:         ReserveSnapshot memory currentSnapshot = reserveSnapshots[snapshotIndex];//@audit 3 SLOADs vs 1 enough
669:         // return the latest snapshot price directly
670:         // if only one snapshot or the timestamp of latest snapshot is earlier than asking for
671:         if (reserveSnapshots.length == 1 || currentSnapshot.timestamp <= baseTimestamp) {//@audit reserveSnapshots.length  SLOAD 2
...
675:         uint256 previousTimestamp = currentSnapshot.timestamp;
676:         uint256 period = _blockTimestamp() - previousTimestamp;
677:         uint256 weightedPrice = currentPrice * period;
678:         while (true) {
...
680:             if (snapshotIndex == 0) {
681:                 return weightedPrice / period;
682:             }
...
684:             snapshotIndex = snapshotIndex - 1; //@audit uncheck (see L680-L682)
685:             currentSnapshot = reserveSnapshots[snapshotIndex];
686:             currentPrice = reserveSnapshots[snapshotIndex].lastPrice; //@audit use currentSnapshot.lastPrice
...
689:             if (currentSnapshot.timestamp <= baseTimestamp) {
...
698:             uint256 timeFraction = previousTimestamp - currentSnapshot.timestamp;
...
701:             previousTimestamp = currentSnapshot.timestamp;

Do not cache `reserveSnapshots[snapshotIndex]` in memory

Here, we need to cache the timestamp field. Copying the struct in memory costs 3 SLOADs.

Cache `reserveSnapshots.length` in memory

This would save 1 SLOAD

Unchecked block L684

This line can't underflow due to the condition L680-L682. Therefore, it should be wrapped in an unchecked block

Use the cache for calculation

As we already have currentSnapshot = reserveSnapshots[snapshotIndex];: use it here: currentPrice = currentSnapshot.lastPrice;

File: ClearingHouse.sol

function _disperseLiquidationFee()

210:     function _disperseLiquidationFee(uint liquidationFee) internal {
211:         if (liquidationFee > 0) {
212:             uint toInsurance = liquidationFee / 2;
213:             marginAccount.transferOutVusd(address(insuranceFund), toInsurance);
214:             marginAccount.transferOutVusd(_msgSender(), liquidationFee - toInsurance); //@audit uncheck (see L212)
215:         }
216:     }

Unchecked block L214

This line can't underflow due to the condition L212. Therefore, it should be wrapped in an unchecked block

File: InsuranceFund.sol

function pricePerShare()

File: InsuranceFund.sol
094:     function pricePerShare() external view returns (uint) {
095:         uint _totalSupply = totalSupply();
096:         uint _balance = balance();
097:         _balance -= Math.min(_balance, pendingObligation); //@audit uncheck
098:         if (_totalSupply == 0 || _balance == 0) 
099:             return PRECISION;
100:         }
101:         return _balance * PRECISION / 
_totalSupply;
102:     }

Unchecked block L97

This line can't underflow for obvious mathematical reasons (_balance substracting at most itself). Therefore, it should be wrapped in an unchecked block

File: Oracle.sol

function getUnderlyingTwapPrice()

Unchecked block L81

This line can't underflow due to L76-L79. Therefore, it should be wrapped in an unchecked block

File: Interfaces.sol

struct Collateral

Tight packing structs to save slots

While this file is out of scope, it deeply impacts MarginAccount.sol.
I suggest going from:

94:     struct Collateral {
95:         IERC20 token; //@audit 20 bytes
96:         uint weight; //@audit 32 bytes
97:         uint8 decimals; //@audit 1 byte
98:     }

to

94:     struct Collateral {
95:         IERC20 token; //@audit 20 bytes
96:         uint8 decimals; //@audit 1 byte
97:         uint weight; //@audit 32 bytes
98:     }

To save 1 slot per array element in MarginAccount.sol's storage

File: MarginAccount.sol

function _getLiquidationInfo()

460:     function _getLiquidationInfo(address trader, uint idx) internal view returns (LiquidationBuffer memory buffer) {
461:         require(idx > VUSD_IDX && idx < supportedCollateral.length, "collateral not seizable");
462:         (buffer.status, buffer.repayAble, buffer.incentivePerDollar) = isLiquidatable(trader, false);
463:         if (buffer.status == IMarginAccount.LiquidationStatus.IS_LIQUIDATABLE) {
464:             Collateral memory coll = supportedCollateral[idx];//@audit 3 SLOADs vs 2 enough
465:             buffer.priceCollateral = oracle.getUnderlyingPrice(address(coll.token)).toUint256();
466:             buffer.decimals = coll.decimals;
467:         }
468:     }

Do not cache `supportedCollateral[idx]` in memory

Here, we need Collateral storage coll = supportedCollateral[idx];. Copying the struct in memory costs 3 SLOADs.

function _transferOutVusd()

Unchecked block L588

This line can't underflow due to L583. Therefore, it should be wrapped in an unchecked block

File: VUSD.sol

function processWithdrawals()

53:     function processWithdrawals() external {
54:         uint reserve = reserveToken.balanceOf(address(this));
55:         require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance'); //@audit start SLOAD 1
56:         uint i = start;//@audit start SLOAD 2
57:         while (i < withdrawals.length && (i - start) <= maxWithdrawalProcesses) { //@audit uncheck whole //@audit start SLOAD 3
58:             Withdrawal memory withdrawal = withdrawals[i]; //@audit-ok
59:             if (reserve < withdrawal.amount) {
60:                 break;
61:             }
62:             reserveToken.safeTransfer(withdrawal.usr, withdrawal.amount);
63:             reserve -= withdrawal.amount;  //@audit uncheck (see L59-L61)
64:             i += 1;
65:         }
66:         start = i;
67:     }

Unchecked block L57-L65

The whole while-loop can't underflow. Therefore, it should be wrapped in an unchecked block

Cache `start` in memory

Cache start in memory as initialStart and use it L55 + L57 (compare i to it in the while-loop)

General recommendations

Variables

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

ClearingHouse.sol:122:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:130:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:170:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:194:        for (uint i = 0; i < amms.length; i++) { // liquidate all positions
ClearingHouse.sol:251:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:263:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:277:        for (uint i = 0; i < amms.length; i++) {
InsuranceFund.sol:52:        uint shares = 0;
MarginAccount.sol:31:    uint constant VUSD_IDX = 0;
MarginAccount.sol:331:        for (uint i = 0; i < idxs.length; i++) {
MarginAccount.sol:521:        for (uint i = 0; i < assets.length; i++) {
MarginAccount.sol:552:        for (uint i = 0; i < _collaterals.length; i++) {
MarginAccountHelper.sol:13:    uint constant VUSD_IDX = 0;

I suggest removing explicit initializations for default values.

Pre-increments cost less gas compared to post-increments

Comparisons

`> 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

> 0 in require statements are used in the following location(s):

AMM.sol:487:        require(baseAssetQuantity > 0, "VAMM._long: baseAssetQuantity is <= 0");
AMM.sol:511:        require(baseAssetQuantity < 0, "VAMM._short: baseAssetQuantity is >= 0");
ClearingHouse.sol:51:        require(_maintenanceMargin > 0, "_maintenanceMargin < 0");
MarginAccount.sol:150:        require(amount > 0, "Add non-zero margin");
Oracle.sol:153:        require(_round > 0, "Not enough history");

I suggest you change > 0 with != 0 in require statements. Also, enable the Optimizer.

For-Loops

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:

ClearingHouse.sol:122:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:130:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:170:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:194:        for (uint i = 0; i < amms.length; i++) { // liquidate all positions
ClearingHouse.sol:251:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:263:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:277:        for (uint i = 0; i < amms.length; i++) {
MarginAccount.sol:331:        for (uint i = 0; i < idxs.length; i++) {
MarginAccount.sol:373:        for (uint i = 1 /* skip vusd */; i < assets.length; i++) {
MarginAccount.sol:521:        for (uint i = 0; i < assets.length; i++) {
MarginAccount.sol:552:        for (uint i = 0; i < _collaterals.length; i++) {

`++i` costs less gas compared to `i++`

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

ClearingHouse.sol:122:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:130:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:170:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:194:        for (uint i = 0; i < amms.length; i++) { // liquidate all positions
ClearingHouse.sol:251:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:263:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:277:        for (uint i = 0; i < amms.length; i++) {
MarginAccount.sol:331:        for (uint i = 0; i < idxs.length; i++) {
MarginAccount.sol:373:        for (uint i = 1 /* skip vusd */; i < assets.length; i++) {
MarginAccount.sol:521:        for (uint i = 0; i < assets.length; i++) {
MarginAccount.sol:552:        for (uint i = 0; i < _collaterals.length; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Instances include:

ClearingHouse.sol:122:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:130:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:170:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:194:        for (uint i = 0; i < amms.length; i++) { // liquidate all positions
ClearingHouse.sol:251:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:263:        for (uint i = 0; i < amms.length; i++) {
ClearingHouse.sol:277:        for (uint i = 0; i < amms.length; i++) {
MarginAccount.sol:331:        for (uint i = 0; i < idxs.length; i++) {
MarginAccount.sol:373:        for (uint i = 1 /* skip vusd */; i < assets.length; i++) {
MarginAccount.sol:521:        for (uint i = 0; i < assets.length; i++) {
MarginAccount.sol:552:        for (uint i = 0; i < _collaterals.length; i++) {

The code would go from:

for (uint256 i; i < numIterations; i++) {  
 // ...  
}

to:

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}

The risk of overflow is inexistant for a uint256 here.

Arithmetics

Shift Right instead of Dividing by 2

A division by 2 can be calculated by shifting one to the right.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

I suggest replacing / 2 with >> 1 here:

ClearingHouse.sol:212:            uint toInsurance = liquidationFee / 2;

Errors

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes are here:

AMM.sol:487:        require(baseAssetQuantity > 0, "VAMM._long: baseAssetQuantity is <= 0");
AMM.sol:511:        require(baseAssetQuantity < 0, "VAMM._short: baseAssetQuantity is >= 0");
ClearingHouse.sol:84:            require(isAboveMinAllowableMargin(trader), "CH: Below Minimum Allowable Margin");
ClearingHouse.sol:101:        require(isAboveMinAllowableMargin(maker), "CH: Below Minimum Allowable Margin");
MarginAccount.sol:174:        require(margin[VUSD_IDX][trader] >= 0, "Cannot remove margin when vusd balance is negative");
MarginAccount.sol:354:        require(notionalPosition == 0, "Liquidate positions before settling bad debt");
MarginAccount.sol:453:        require(repay <= maxRepay, "Need to repay more to seize that much");

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.

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:

legos/Governable.sol:11:        require(msg.sender == governance, "ONLY_GOVERNANCE");
AMM.sol:84:        require(msg.sender == clearingHouse, "Only clearingHouse");
AMM.sol:89:        require(msg.sender == address(vamm), "Only VAMM");
AMM.sol:119:        require(ammState == AMMState.Active, "AMM.openPosition.not_active");
AMM.sol:186:        require(ammState != AMMState.Inactive, "AMM.addLiquidity.amm_inactive");
AMM.sol:296:        require(abs(positionSize) >= abs(baseAssetQuantity), "AMM.ONLY_REDUCE_POS");
AMM.sol:348:        require(_blockTimestamp() >= nextFundingTime, "settle funding too early");
AMM.sol:487:        require(baseAssetQuantity > 0, "VAMM._long: baseAssetQuantity is <= 0");
AMM.sol:511:        require(baseAssetQuantity < 0, "VAMM._short: baseAssetQuantity is >= 0");
AMM.sol:723:        require(ammState != _state, "AMM.setAmmState.sameState");
ClearingHouse.sol:51:        require(_maintenanceMargin > 0, "_maintenanceMargin < 0");
ClearingHouse.sol:75:        require(baseAssetQuantity != 0, "CH: baseAssetQuantity == 0");
ClearingHouse.sol:84:            require(isAboveMinAllowableMargin(trader), "CH: Below Minimum Allowable Margin");
ClearingHouse.sol:101:        require(isAboveMinAllowableMargin(maker), "CH: Below Minimum Allowable Margin");
ClearingHouse.sol:120:        require(address(trader) != address(0), 'CH: 0x0 trader Address');
ClearingHouse.sol:154:        require(!isMaker(trader), 'CH: Remove Liquidity First');
ClearingHouse.sol:164:        require(
ClearingHouse.sol:189:        require(_calcMarginFraction(trader, false /* check funding payments again */) < maintenanceMargin, "Above Maintenance Margin");
InsuranceFund.sol:30:        require(msg.sender == address(marginAccount), "IF.only_margin_account");
InsuranceFund.sol:42:        require(pendingObligation == 0, "IF.deposit.pending_obligations");
InsuranceFund.sol:64:        require(pendingObligation == 0, "IF.withdraw.pending_obligations");
MarginAccount.sol:115:        require(_msgSender() == address(clearingHouse), "Only clearingHouse");
MarginAccount.sol:150:        require(amount > 0, "Add non-zero margin");
MarginAccount.sol:174:        require(margin[VUSD_IDX][trader] >= 0, "Cannot remove margin when vusd balance is negative");
MarginAccount.sol:175:        require(margin[idx][trader] >= amount.toInt256(), "Insufficient balance");
MarginAccount.sol:180:        require(clearingHouse.isAboveMinAllowableMargin(trader), "MA.removeMargin.Below_MM");
MarginAccount.sol:354:        require(notionalPosition == 0, "Liquidate positions before settling bad debt");
MarginAccount.sol:357:        require(getSpotCollateralValue(trader) < 0, "Above bad debt threshold");
MarginAccount.sol:362:        require(vusdBal < 0, "Nothing to repay");
MarginAccount.sol:438:        require(seized >= minSeizeAmount, "Not seizing enough");
MarginAccount.sol:453:        require(repay <= maxRepay, "Need to repay more to seize that much");
MarginAccount.sol:461:        require(idx > VUSD_IDX && idx < supportedCollateral.length, "collateral not seizable");
MarginAccount.sol:549:        require(_weight <= PRECISION, "weight > 1e6");
MarginAccount.sol:553:            require(address(_collaterals[i].token) != _coin, "collateral exists");
MarginAccount.sol:601:        require(_liquidationIncentive <= PRECISION / 10, "MA.syncDeps.LI_GT_10_percent");
MarginAccount.sol:603:        require(registry.marginAccount() == address(this), "Incorrect setup");
MarginAccount.sol:617:        require(_weight <= PRECISION, "weight > 1e6");
MarginAccount.sol:618:        require(idx < supportedCollateral.length, "Collateral not supported");
MinimalForwarder.sol:15:        require(success, string(abi.encodePacked("META_EXEC_FAILED: ", returnData)));
Oracle.sol:48:        require(intervalInSeconds != 0, "interval can't be 0");
Oracle.sol:153:        require(_round > 0, "Not enough history");
Oracle.sol:157:        require(_addr != address(0), "empty address");
VUSD.sol:33:        require(_reserveToken != address(0), "vUSD: null _reserveToken");
VUSD.sol:55:        require(reserve >= withdrawals[start].amount, 'Cannot process withdrawals at this time: Not enough balance');

I suggest replacing revert strings with custom errors.

The text was updated successfully, but these errors were encountered:

atvanguard · 2022-02-26T08:03:16Z

Amazing report! ⭐

moose-code · 2022-03-05T15:13:14Z

Really detailed and well constructed report 💯

code423n4 added bug Something isn't working G (Gas Optimization) labels Feb 23, 2022

code423n4 added a commit that referenced this issue Feb 23, 2022

Dravee issue #135

e84b901

atvanguard added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Feb 26, 2022

atvanguard added 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 Feb 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gas Optimizations #135

Gas Optimizations #135

code423n4 commented Feb 23, 2022

atvanguard commented Feb 26, 2022

moose-code commented Mar 5, 2022

Gas Optimizations #135

Gas Optimizations #135

Comments

code423n4 commented Feb 23, 2022

Gas Report

Foreword

Summary

File: AMM.sol

function initialize()

Use calldata instead of memory for string _name

function openPosition()

Do not cache positions[trader] in memory

function liquidatePosition()

Do not cache positions[trader] in memory

function removeLiquidity()

Do not cache positions[maker] in memory

Do not cache makers[maker] in memory

function getNotionalPositionAndUnrealizedPnl()

Do not cache positions[trader] in memory

Do not cache makers[trader] in memory

function getPendingFundingPayment()

Do not cache positions[trader] in memory

Do not cache makers[trader] in memory

function getTakerNotionalPositionAndUnrealizedPnl()

Do not cache positions[trader] in memory

function _emitPositionChanged()

Do not cache positions[trader] in memory

function _openReversePosition()

Do not cache positions[trader] in memory

Unchecked block L597

function _calcTwap()

Do not cache reserveSnapshots[snapshotIndex] in memory

Cache reserveSnapshots.length in memory

Unchecked block L684

Use the cache for calculation

File: ClearingHouse.sol

function _disperseLiquidationFee()

Unchecked block L214

File: InsuranceFund.sol

function pricePerShare()

Unchecked block L97

File: Oracle.sol

function getUnderlyingTwapPrice()

Unchecked block L81

File: Interfaces.sol

struct Collateral

Tight packing structs to save slots

File: MarginAccount.sol

function _getLiquidationInfo()

Do not cache supportedCollateral[idx] in memory

function _transferOutVusd()

Unchecked block L588

File: VUSD.sol

function processWithdrawals()

Unchecked block L57-L65

Cache start in memory

General recommendations

Variables

No need to explicitly initialize variables with default values

Pre-increments cost less gas compared to post-increments

Comparisons

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

For-Loops

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

++i costs less gas compared to i++

Increments can be unchecked

Arithmetics

Shift Right instead of Dividing by 2

Errors

Reduce the size of error messages (Long revert Strings)

Use Custom Errors instead of Revert Strings to save Gas

atvanguard commented Feb 26, 2022

moose-code commented Mar 5, 2022

Use `calldata` instead of `memory` for `string _name`

Do not cache `positions[trader]` in memory

Do not cache `positions[trader]` in memory

Do not cache `positions[maker]` in memory

Do not cache `makers[maker]` in memory

Do not cache `positions[trader]` in memory

Do not cache `makers[trader]` in memory

Do not cache `positions[trader]` in memory

Do not cache `makers[trader]` in memory

Do not cache `positions[trader]` in memory

Do not cache `positions[trader]` in memory

Do not cache `positions[trader]` in memory

Do not cache `reserveSnapshots[snapshotIndex]` in memory

Cache `reserveSnapshots.length` in memory

Do not cache `supportedCollateral[idx]` in memory

Cache `start` in memory

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

`++i` costs less gas compared to `i++`