prePO contest - Dravee's results

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • File: PrePOMarket.sol
    • Tighly pack storage variables
    • function redeem()
      • Cache _finalLongPrice
  • File: CollateralDepositRecord.sol
    • function recordDeposit()
      • Cache _amount + _globalDepositAmount
      • Cache _amount + _accountToNetDeposit[_sender]
    • function recordWithdrawal()
      • Cache _globalDepositAmount
      • Cache _accountToNetDeposit
  • General recommendations
    • Storage Variables
      • Emitting storage values instead of caching of function arguments
      • Incrementing before emitting a storage value instead of emitting the increment's result
      • Some storage variables should be immutable
      • Help the optimizer by declaring a storage variable instead of repeatedly fetching the value
    • 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
      • Uncheck calculations to save gas when an overflow/underflow is impossible
    • Errors
      • Use Custom Errors instead of Revert Strings to save Gas

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.

File: PrePOMarket.sol

Tighly pack storage variables

Here, storage variables can be tightly packed from:

File: PrePOMarket.sol
11:     address private _treasury; //@audit 20 bytes
12: 
13:     IERC20 private immutable _collateral;
14:     ILongShortToken private immutable _longToken;
15:     ILongShortToken private immutable _shortToken;
16: 
17:     uint256 private immutable _floorLongPrice;
18:     uint256 private immutable _ceilingLongPrice;
19:     uint256 private _finalLongPrice;//@audit 32 bytes
20: 
21:     uint256 private immutable _floorValuation;
22:     uint256 private immutable _ceilingValuation;
23: 
24:     uint256 private _mintingFee;
25:     uint256 private _redemptionFee;//@audit 32 bytes
26: 
27:     uint256 private immutable _expiryTime;
28: 
29:     bool private _publicMinting; //@audit 1 byte, but taking 1 slot. Can be tightly packed

to

File: PrePOMarket.sol
11:     address private _treasury; //@audit 20 bytes
12:     
13:     bool private _publicMinting; //@audit 1 byte
14: 
15:     IERC20 private immutable _collateral;
16:     ILongShortToken private immutable _longToken;
17:     ILongShortToken private immutable _shortToken;
18: 
19:     uint256 private immutable _floorLongPrice;
20:     uint256 private immutable _ceilingLongPrice;
21:     uint256 private _finalLongPrice;//@audit 32 bytes
22: 
23:     uint256 private immutable _floorValuation;
24:     uint256 private immutable _ceilingValuation;
25: 
26:     uint256 private _mintingFee;
27:     uint256 private _redemptionFee;//@audit 32 bytes
28: 
29:     uint256 private immutable _expiryTime;

Which would save 1 storage slot.

function redeem()

Cache _finalLongPrice

Caching this in memory can save around 2 SLOADs (around 200 gas)

File: PrePOMarket.sol
145:         if (_finalLongPrice <= MAX_PRICE) { //@audit _finalLongPrice SLOAD 1
146:             uint256 _shortPrice = MAX_PRICE - _finalLongPrice; //@audit _finalLongPrice SLOAD 2
147:             _collateralOwed =
148:                 (_finalLongPrice * _longAmount + _shortPrice * _shortAmount) / //@audit _finalLongPrice SLOAD 3
149:                 MAX_PRICE;

File: CollateralDepositRecord.sol

function recordDeposit()

File: CollateralDepositRecord.sol
24:     function recordDeposit(address _sender, uint256 _amount)
25:         external
26:         override
27:         onlyAllowedHooks
28:     {
29:         require(
30:             _amount + _globalDepositAmount <= _globalDepositCap, //@audit 1st _amount + _globalDepositAmount
31:             "Global deposit cap exceeded"
32:         );
33:         require(
34:             _amount + _accountToNetDeposit[_sender] <= _accountDepositCap, //@audit 1st _amount + _accountToNetDeposit[_sender]
35:             "Account deposit cap exceeded"
36:         );
37:         _globalDepositAmount += _amount; //@audit 2nd _amount + _globalDepositAmount
38:         _accountToNetDeposit[_sender] += _amount; //@audit 2nd _amount + _accountToNetDeposit[_sender]
39:     }

Cache _amount + _globalDepositAmount

Cache _amount + _accountToNetDeposit[_sender]

Optimized code:

File: CollateralDepositRecord.sol
24:     function recordDeposit(address _sender, uint256 _amount)
25:         external
26:         override
27:         onlyAllowedHooks
28:     {
29:         uint256 _newGlobalDepositAmount = _amount + _globalDepositAmount;
30:         uint256 _newAccountToNetDeposit = _amount + _accountToNetDeposit[_sender];
31:         require(
32:             _newGlobalDepositAmount <= _globalDepositCap,
33:             "Global deposit cap exceeded"
34:         );
35:         require(
36:             _newAccountToNetDeposit <= _accountDepositCap,
37:             "Account deposit cap exceeded"
38:         );
39:         _globalDepositAmount = _newGlobalDepositAmount;
40:         _accountToNetDeposit[_sender] = _newAccountToNetDeposit; 
41:     }

function recordWithdrawal()

41:     function recordWithdrawal(address _sender, uint256 _amount)
42:         external
43:         override
44:         onlyAllowedHooks
45:     {
46:         if (_globalDepositAmount > _amount) { //@audit _globalDepositAmount SLOAD 1
47:             _globalDepositAmount -= _amount; //@audit _globalDepositAmount SLOAD 2
48:         } else {
49:             _globalDepositAmount = 0;
50:         }
51:         if (_accountToNetDeposit[_sender] > _amount) { //@audit _accountToNetDeposit SLOAD 1
52:             _accountToNetDeposit[_sender] -= _amount; //@audit _accountToNetDeposit SLOAD 2
53:         } else {
54:             _accountToNetDeposit[_sender] = 0;
55:         }
56:     }

Cache _globalDepositAmount

Cache _accountToNetDeposit

Optimized code:

File: CollateralDepositRecord.sol
41:     function recordWithdrawal(address _sender, uint256 _amount)
42:         external
43:         override
44:         onlyAllowedHooks
45:     {
46:         uint256 __globalDepositAmount = _globalDepositAmount;
47:         if (__globalDepositAmount > _amount) {
48:             _globalDepositAmount = __globalDepositAmount - _amount;
49:         } else {
50:             _globalDepositAmount = 0;
51:         }
52:         uint256 __accountToNetDeposit = _accountToNetDeposit[_sender];
53:         if (__accountToNetDeposit > _amount) { 
54:             _accountToNetDeposit[_sender] = __accountToNetDeposit - _amount;
55:         } else {
56:             _accountToNetDeposit[_sender] = 0;
57:         }
58:     }

General recommendations

Storage Variables

Emitting storage values instead of caching of function arguments

Emitting a storage value can be avoided to save a SLOAD at these places by using the function argument instead:

contracts/core/AccountAccessController.sol:
  96          _root = _newRoot;
  97:         emit RootChanged(_root); //@audit should emit _newRoot

contracts/core/Collateral.sol:
  191          _strategyController = _newStrategyController;
  192:         emit StrategyControllerChanged(address(_strategyController));//@audit should emit address(_strategyController)

  200          _delayedWithdrawalExpiry = _newDelayedWithdrawalExpiry;
  201:         emit DelayedWithdrawalExpiryChanged(_delayedWithdrawalExpiry); //@audit should emit address(_newDelayedWithdrawalExpiry)

  210          _mintingFee = _newMintingFee;
  211:         emit MintingFeeChanged(_mintingFee); //@audit should emit address(_newMintingFee)

  220          _redemptionFee = _newRedemptionFee;
  221:         emit RedemptionFeeChanged(_redemptionFee); //@audit should emit address(_newRedemptionFee)

  229          _depositHook = _newDepositHook;
  230:         emit DepositHookChanged(address(_depositHook)); //@audit should emit address(_depositHook)

  238          _withdrawHook = _newWithdrawHook;
  239:         emit WithdrawHookChanged(address(_withdrawHook)); //@audit should emit address(_newWithdrawHook)

contracts/core/CollateralDepositRecord.sol:
  63          _globalDepositCap = _newGlobalDepositCap;
  64:         emit GlobalDepositCapChanged(_globalDepositCap); //@audit should emit _newGlobalDepositCap

Incrementing before emitting a storage value instead of emitting the increment's result

These can be optimized:

contracts/core/AccountAccessController.sol:
   35          _blockedAccountsIndex++;
   36:         emit BlockedAccountsCleared(_blockedAccountsIndex); //@audit should use ++_blockedAccountsIndex

  101          _allowedAccountsIndex++;
  102:         emit AllowedAccountsCleared(_allowedAccountsIndex); //@audit should use ++_allowedAccountsIndex

to

contracts/core/AccountAccessController.sol:
   35:         emit BlockedAccountsCleared(++_blockedAccountsIndex);

  101:         emit AllowedAccountsCleared(++_allowedAccountsIndex);

Some storage variables should be immutable

Marking these as immutable (as they never change outside the constructor) would avoid them taking space in the storage:

contracts/core/Collateral.sol:
  23:     address private _treasury;//@audit should be immutable
  26:     IERC20Upgradeable private _baseToken; //@audit should be immutable

contracts/core/DepositHook.sol:
  11:     IAccountAccessController private _accountAccessController; //@audit should be immutable
  12:     ICollateralDepositRecord private _depositRecord; //@audit should be immutable

contracts/core/WithdrawHook.sol:
  10:     ICollateralDepositRecord private _depositRecord; //@audit should be immutable

Help the optimizer by declaring a storage variable instead of repeatedly fetching the value

To help the optimizer, go from:

File: AccountAccessController.sol
61:     function allowSelf(bytes32[] calldata _proof) external override {
62:         require(
63:             _allowedAccounts[_allowedAccountsIndex][msg.sender] == false, //@audit help the optimizer + use just require(!_allowedAccounts[_allowedAccountsIndex][msg.sender]) as this here is a comparison to a constant
...
69:         _allowedAccounts[_allowedAccountsIndex][msg.sender] = true; //@audit help the optimizer

to

File: AccountAccessController.sol
     function allowSelf(bytes32[] calldata _proof) external override {
         bool storage _accountAllowed = _allowedAccounts[_allowedAccountsIndex][msg.sender];
         require(
             _accountAllowed == false,
...
         _accountAllowed = true;

Also here, use require(!_allowedAccounts[_allowedAccountsIndex][msg.sender]) at L63 to avoid the cost of a comparison to a constant.

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:

core/AccountAccessController.sol:44:        for (uint256 _i = 0; _i < _accounts.length; _i++) {
core/AccountAccessController.sol:55:        for (uint256 _i = 0; _i < _accounts.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:

core/AccountAccessController.sol:35:        _blockedAccountsIndex++;
core/AccountAccessController.sol:44:        for (uint256 _i = 0; _i < _accounts.length; _i++) {
core/AccountAccessController.sol:55:        for (uint256 _i = 0; _i < _accounts.length; _i++) {
core/AccountAccessController.sol:101:        _allowedAccountsIndex++;

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:

core/AccountAccessController.sol:44:        for (uint256 _i = 0; _i < _accounts.length; _i++) {
core/AccountAccessController.sol:55:        for (uint256 _i = 0; _i < _accounts.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

Uncheck calculations to save gas when an overflow/underflow is impossible

Instances include:

contracts/core/Collateral.sol:
   71          require(_amountToDeposit > _fee, "Deposit amount too small");
   72          _baseToken.safeTransfer(_treasury, _fee);
   73:         _amountToDeposit -= _fee; //@audit uncheck (see L71)

  169          require(_amountWithdrawn > _fee, "Withdrawal amount too small");
  170          _baseToken.safeTransfer(_treasury, _fee);
  171:         _amountWithdrawn -= _fee; //@audit uncheck (see L169)

contracts/core/CollateralDepositRecord.sol:
  46          if (_globalDepositAmount > _amount) { 
  47:             _globalDepositAmount -= _amount; //@audit uncheck (see L46)

  51          if (_accountToNetDeposit[_sender] > _amount) {
  52:             _accountToNetDeposit[_sender] -= _amount; //@audit uncheck (see L51)

contracts/core/PrePOMarket.sol:
  120         require(_amount > _fee, "Minting amount too small");
  121          _collateral.transferFrom(msg.sender, _treasury, _fee);
  122:         _amount -= _fee; //@audit uncheck (see L120)

  167          require(_collateralOwed > _fee, "Redemption amount too small");
  168          _collateral.transfer(_treasury, _fee);
  169:         _collateralOwed -= _fee; //@audit uncheck (see L167)

Errors

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:

core/AccountAccessController.sol:62:        require(
core/AccountAccessController.sol:68:        require(MerkleProof.verify(_proof, _root, _leaf), "Invalid proof");
core/Collateral.sol:58:        require(_depositsAllowed, "Deposits not allowed");
core/Collateral.sol:71:        require(_amountToDeposit > _fee, "Deposit amount too small");
core/Collateral.sol:101:        require(balanceOf(msg.sender) >= _amount, "Insufficient balance");
core/Collateral.sol:118:        require(
core/Collateral.sol:124:        require(
core/Collateral.sol:128:        require(
core/Collateral.sol:143:        require(_withdrawalsAllowed, "Withdrawals not allowed");
core/Collateral.sol:169:        require(_amountWithdrawn > _fee, "Withdrawal amount too small");
core/Collateral.sol:209:        require(_newMintingFee <= FEE_LIMIT, "Exceeds fee limit");
core/Collateral.sol:219:        require(_newRedemptionFee <= FEE_LIMIT, "Exceeds fee limit");
core/CollateralDepositRecord.sol:15:        require(_allowedHooks[msg.sender], "Caller not allowed");
core/CollateralDepositRecord.sol:29:        require(
core/CollateralDepositRecord.sol:33:        require(
core/DepositHook.sol:22:        require(msg.sender == _vault, "Caller is not the vault");
core/DepositHook.sol:31:        require(
core/PrePOMarket.sol:58:        require(
core/PrePOMarket.sol:62:        require(_newExpiryTime > block.timestamp, "Invalid expiry");
core/PrePOMarket.sol:63:        require(_newMintingFee <= FEE_LIMIT, "Exceeds fee limit");
core/PrePOMarket.sol:64:        require(_newRedemptionFee <= FEE_LIMIT, "Exceeds fee limit");
core/PrePOMarket.sol:65:        require(_newCeilingLongPrice <= MAX_PRICE, "Ceiling cannot exceed 1");
core/PrePOMarket.sol:108:            require(_publicMinting, "Public minting disabled");
core/PrePOMarket.sol:110:        require(_finalLongPrice > MAX_PRICE, "Market ended");
core/PrePOMarket.sol:111:        require(
core/PrePOMarket.sol:120:        require(_amount > _fee, "Minting amount too small");
core/PrePOMarket.sol:135:        require(
core/PrePOMarket.sol:139:        require(
core/PrePOMarket.sol:151:            require(
core/PrePOMarket.sol:167:        require(_collateralOwed > _fee, "Redemption amount too small");
core/PrePOMarket.sol:185:        require(
core/PrePOMarket.sol:189:        require(
core/PrePOMarket.sol:202:        require(_newMintingFee <= FEE_LIMIT, "Exceeds fee limit");
core/PrePOMarket.sol:212:        require(_newRedemptionFee <= FEE_LIMIT, "Exceeds fee limit");
core/PrePOMarketFactory.sol:55:        require(_validCollateral[_collateral], "Invalid collateral");
core/SingleStrategyController.sol:22:        require(msg.sender == _vault, "Caller is not the vault");
core/SingleStrategyController.sol:27:        require(address(_token) != address(0), "Zero address");
core/WithdrawHook.sol:17:        require(msg.sender == _vault, "Caller is not the vault");

I suggest replacing revert strings with custom errors.