Platform: Code4rena
Start Date: 17/02/2022
Pot Size: $75,000 USDC
Total HM: 20
Participants: 39
Period: 7 days
Judges: moose-code, JasoonS
Total Solo HM: 13
Id: 89
League: ETH
Rank: 31/39
Findings: 1
Award: $227.49
🌟 Selected for report: 0
🚀 Solo Findings: 0
227.4932 USDC - $227.49
The local variable used as for loop index need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.
function settleFunding() override external whenNotPaused { for (uint i = 0; i < amms.length; i++) { amms[i].settleFunding(); } }
The same situation are in other scope contracts where loops use.
Remix
Remove explicit 0 initialization of for loop index variable.
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.
function settleFunding() override external whenNotPaused { for (uint i = 0; i < amms.length; i++) { amms[i].settleFunding(); } }
The same situation are in other scope contracts where loops use.
Manual
Caching len = amms.length and using the len instead will save gas.
There is no risk of overflow caused by increamenting the iteration index in for loops. Increments perform overflow checks that are not necessary in this case.
function settleFunding() override external whenNotPaused { for (uint i = 0; i < amms.length; i++) { amms[i].settleFunding(); } }
The same situation are in other scope contracts where loops use.
Remix
Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.
There are several other places throughout the codebase where the same optimization can be used.
Shorten the revert strings to fit in 32 bytes.
!= 0 is a cheaper operation compared to > 0, when dealing with uint.
https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L574 https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/ClearingHouse.sol#L211
There are several other places throughout the codebase where the same optimization can be used.
Remix
In the AMM.sol, declaring the type bytes32 can save gas.
https://medium.com/layerx/how-to-reduce-gas-cost-in-solidity-f2e5321e0395#2a78
ReserveSnapshot struct in AMM.sol can be optimized to reduce 2 storage slot
struct ReserveSnapshot { uint256 lastPrice; uint256 timestamp; uint256 blockNumber; }
timestamp and blockNumber store block numbers, and 2^128 is be enough for a very long time.
Change the struct as suggested above
struct ReserveSnapshot { uint256 lastPrice; uint128 timestamp; uint128 blockNumber; }
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/AMM.sol#L726 https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/ClearingHouse.sol#L212 https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L175-L177
require(margin[idx][trader] >= amount.toInt256(), "Insufficient balance"); margin[idx][trader] -= amount.toInt256();
https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L73 https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L81
uint256 minNextValidFundingTime = _blockTimestamp() + fundingBufferPeriod;
Consider using 'unchecked' where it is safe to do so.
Some of the variable can be cached to slightly reduce gas usage.
vamm can be cached.
https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/AMM.sol#L188-L195
_blockTimestamp() can be cahed
vusd can be cashed
Remix
Consider caching those variable for read and make sure write back to storage
The require statement in the function initialize() can be placed earlier to reduce gas usage on revert.
function initialize( address _governance, address _insuranceFund, address _marginAccount, address _vusd, int256 _maintenanceMargin, int256 _minAllowableMargin, uint _tradeFee, uint _liquidationPenalty ) external initializer { _setGovernace(_governance); insuranceFund = IInsuranceFund(_insuranceFund); marginAccount = IMarginAccount(_marginAccount); vusd = VUSD(_vusd); require(_maintenanceMargin > 0, "_maintenanceMargin < 0"); maintenanceMargin = _maintenanceMargin; minAllowableMargin = _minAllowableMargin; tradeFee = _tradeFee; liquidationPenalty = _liquidationPenalty; }
function getUnderlyingTwapPrice(address underlying, uint256 intervalInSeconds) virtual public view returns (int256) { if (stablePrice[underlying] != 0) { return stablePrice[underlying]; } AggregatorV3Interface aggregator = AggregatorV3Interface(chainLinkAggregatorMap[underlying]); requireNonEmptyAddress(address(aggregator)); require(intervalInSeconds != 0, "interval can't be 0");
Remix
Relocate the said require statement
The constant variable is being assigned its default value which is unnecessary.
uint constant VUSD_IDX = 0;
Remove the assignment.
Checking if _amount > 0 before making the external call to vusd.safeTransferFrom() can save 2600 gas by avoiding the external call in such situations.
Add check
function decimals() just returns a constant of uint8(6). To save some gas and improve the readability this can be extracted to a constant variable and used where necessary.
function decimals() public pure override returns (uint8) { return 6; }
Where possible, use equivalent function parameters or local variables in event emits instead of state variables to prevent expensive SLOADs. Post-Berlin, SLOADs on state variables accessed first-time in a transaction increased from 800 gas to 2100, which is a 2.5x increase.
https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L205 https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/MarginAccount.sol#L381
function realizePnL(address trader, int256 realizedPnl) override external onlyClearingHouse { if (realizedPnl != 0) { margin[VUSD_IDX][trader] += realizedPnl; emit PnLRealized(trader, realizedPnl, _blockTimestamp()); }
Use equivalent function parameters or local variables in event emits instead of state variables.
_blockTimestamp() returns the block.timestamp value in AMM contract. This internal call only to get value of block.timestamp seems unnecessary because there isn’t any other way of getting current time on the blockchain which justifies moving this to a separate function for modularity.
Adds an additional jump and other supporting bytecode of making the internal call which increase gas usage unnecessarily.
function _blockTimestamp() internal view virtual returns (uint256) { return block.timestamp; }
Recommended Mitigation Steps Use block.timestamp directly to save a little gas by avoiding this unnecessary indirection.
Each function part of contract's external interface is part of the function dispatch, i.e., every time a contract is called, it goes through a switch statement (a set of eq ... JUMPI blocks in EVM) matching the selector of each externally available functions with the chosen function selector (the first 4 bytes of calldata). This means that any unnecessary function that is part of contract's external interface will lead to more gas for (almost) every single function calls to the contract. There are several cases where constants were made public. This is unnecessary; the constants can simply be readfrom the verified contract, i.e., it is unnecessary to expose it with a public function.
This does not directly impact the smart contract in anyway besides cost. This is a gas optimization to reduce cost of smart contract.
The function decimals() public could be set external instead of public.
Custom errors from Solidity 0.8.4 are cheaper than revert strings.
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).
Replace revert strings with custom errors.
More expensive gas usage
require(idx > VUSD_IDX && idx < supportedCollateral.length, "collateral not seizable");
Instead of using operator && on single require check using double require check can save more gas
10 ** DECIMALS for constantMore expensive gas usage
uint8 constant DECIMALS = 6; uint constant PRECISION = 10 ** DECIMALS;
Change to:
uint constant PRECISION = 1e6;
#0 - atvanguard
2022-02-26T08:14:32Z
Good report.
#1 - moose-code
2022-03-05T15:18:22Z
Like the attention to struct packing. A uint32 timestamp lasts till 2106 and allows for some nice packing