Gas Optimizations

[code423n4]

Title: Storage double reading. Could save SLOAD
Severity: GAS

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the
overall gas uses. The following is a list of functions and the storage variables that you read twice:

    PrePOMarket.sol: FEE_LIMIT is read twice in constructor

Title: Use calldata instead of memory
Severity: GAS

Use calldata instead of memory for function parameters
In some cases, having function arguments in calldata instead of
memory is more optimal.

    LongShortToken.constructor (symbol_)
    LongShortToken.constructor (name_)

Title: Rearrange state variables
Severity: GAS

You can change the order of the storage variables to decrease memory uses.

In PrePOMarket.sol,rearranging the storage fields can optimize to: 15 slots from: 16 slots.
The new order of types (you choose the actual variables):
1. IERC20
2. ILongShortToken
3. ILongShortToken
4. uint256
5. uint256
6. uint256
7. uint256
8. uint256
9. uint256
10. uint256
11. uint256
12. uint256
13. uint256
14. uint256
15. address
16. bool

Title: Inline one time use functions
Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    Collateral.sol, _processDelayedWithdrawal

Title: Unnecessary index init
Severity: GAS

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas.
It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    AccountAccessController.sol, 44
    AccountAccessController.sol, 55

Title: Unused imports
Severity: GAS

In the following files there are contract imports that aren't used
Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    LongShortToken.sol, line 5, import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

Title: Unnecessary equals boolean
Severity: GAS

Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

    AccountAccessController.sol, 63: _allowedAccounts[_allowedAccountsIndex][msg.sender] == false,

Title: Prefix increments are cheaper than postfix increments
Severity: GAS

Prefix increments are cheaper than postfix increments.
Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change
There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)).
But increments perform overflow checks that are not necessary in this case.
These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: AccountAccessController.sol, _i, 44
    change to prefix increment and unchecked: AccountAccessController.sol, _i, 55

Title: Internal functions to private
Severity: GAS

The following functions could be set private to save gas and improve code quality:

    AccountAccessController.sol, _clearAllowedAccounts
    AccountAccessController.sol, _setRoot
    Collateral.sol, _processDelayedWithdrawal

Title: State variables that could be set immutable
Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    _depositRecord in DepositHook.sol
    _treasury in Collateral.sol
    _baseToken in Collateral.sol
    _accountAccessController in DepositHook.sol
    _depositRecord in WithdrawHook.sol

Title: Unnecessary Reentrancy Guards
Severity: GAS

Where there is onlyOwner or Initializer modifer, the reentrancy gaurd isn't
necessary (unless you don't trust the owner or the deployer, which will lead to full security breakdown of the project and we believe this is not the case)
This is a list we found of such occurrences:

    SingleStrategyController.sol no need both nonReentrant and onlyOwner modifiers in migrate

Title: Unnecessary constructor
Severity: GAS

The following constructors are empty.
(A similar issue code-423n4/2021-11-fei-findings#12)

    AccountAccessController.sol.constructor
    LongShortToken.sol.constructor

Title: Caching array length can save gas
Severity: GAS

Caching the array length is more gas efficient.
This is because access to a local variable in solidity is more efficient than query storage / calldata / memory.
We recommend to change from:

for (uint256 i=0; i<array.length; i++) { ... }

to:

uint len = array.length  
for (uint256 i=0; i<len; i++) { ... }


    AccountAccessController.sol, _accounts, 55
    AccountAccessController.sol, _accounts, 44

Title: Public functions to external
Severity: GAS

The following functions could be set external to save gas and improve code quality.
External call cost is less expensive than of public functions.

    Collateral.sol, initialize

Title: Use != 0 instead of > 0
Severity: GAS

Using != 0 is slightly cheaper than > 0. (see code-423n4/2021-12-maple-findings#75 for similar issue)

    Collateral.sol, 101: change 'balance > 0' to 'balance != 0'
    PrePOMarket.sol, 136: change 'balance > 0' to 'balance != 0'
    PrePOMarket.sol, 140: change 'balance > 0' to 'balance != 0'
    PrePOMarket.sol, 112: change 'balance > 0' to 'balance != 0'

[ramenforbreakfast]

While I have disputes regarding some of the optimizations, this submission is one of the more thorough/well organized gas reports and I will be referring any duplicate mentions of issues in this report back here.