prePO contest - robee's results

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    Collateral.sol, initialize is missing a reentrancy modifier
    Collateral.sol, setWithdrawHook is missing a reentrancy modifier
    PrePOMarket.sol, setFinalLongPrice is missing a reentrancy modifier
    

Title: Init frontrun Severity: Low Risk

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

    Collateral.sol, initialize, 38

Title: Treasury may be address(0) Severity: Low Risk

Make sure the treasury is not address(0).
    
    
    PrePOMarket.sol.setTreasury _newTreasury
    Collateral.sol.initialize _newTreasury

Title: Missing fee parameter validation Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    Collateral.setRedemptionFee (_newRedemptionFee)
    PrePOMarket.constructor (_newRedemptionFee)
    PrePOMarket.setMintingFee (_newMintingFee)
    PrePOMarket.setRedemptionFee (_newRedemptionFee)
    PrePOMarket.constructor (_newMintingFee)
    Collateral.setMintingFee (_newMintingFee)

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    CollateralDepositRecord.sol.recordDeposit _sender
    Collateral.sol._processDelayedWithdrawal _account
    CollateralDepositRecord.sol.setAllowedHook _hook
    

Title: safeApprove of openZeppelin is deprecated Severity: Low Risk

    You use safeApprove of openZeppelin although it's deprecated.
    (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38)
    You should change it to increase/decrease Allowance as OpenZeppilin says
    This appears in the following locations in the code base
    

Deprecated safeApprove in SingleStrategyController.sol line 59: _baseToken.approve(address(_newStrategy), type(uint256).max);

Deprecated safeApprove in Collateral.sol line 75: _baseToken.approve(address(_strategyController), _amountToDeposit);

Deprecated safeApprove in SingleStrategyController.sol line 61: _baseToken.approve(address(_oldStrategy), 0);

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    DepositHook.sol: function hook parameter _initialAmount isn't used. (hook is external)
    WithdrawHook.sol: function hook parameter _initialAmount isn't used. (hook is external)

Title: Div by 0 Severity: Medium Risk

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

    Collateral.sol (L89) _valueBefore might be 0)

Title: Must approve 0 first Severity: Low/Med Risk

Some tokens (like USDT) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.

approve without approving 0 first Collateral.sol, 75, _baseToken.approve(address(_strategyController), _amountToDeposit);

approve without approving 0 first SingleStrategyController.sol, 59, _baseToken.approve(address(_newStrategy), type(uint256).max);

Title: approve return value is ignored Severity: Med Risk

Some tokens don't correctly implement the EIP20 standard and their approve function returns void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert. Tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the mentioned contracts as they revert the transaction because of the missing return value. We recommend using OpenZeppelin’s SafeERC20 versions with the safeApprove function that handle the return value check as well as non-standard-compliant tokens. The list of occurrences in format (solidity file, line number, actual line) Collateral.sol, 75, _baseToken.approve(address(_strategyController), _amountToDeposit);

SingleStrategyController.sol, 59, _baseToken.approve(address(_newStrategy), type(uint256).max);

SingleStrategyController.sol, 61, _baseToken.approve(address(_oldStrategy), 0);

Title: Anyone can withdraw others Severity: Low Risk

Anyone can withdraw users shares. Although we think that they are sent to the right address, it is still 1) not the desired behavior 2) can be dangerous if the receiver is a smart contract 3) the receiver may not know someone withdraw him

    CollateralDepositRecord.recordWithdrawal
    Collateral.setDelayedWithdrawalExpiry

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 https://github.com/code-423n4/2021-11-fei-findings/issues/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 https://github.com/code-423n4/2021-12-maple-findings/issues/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'