QA Report

[code423n4]

Low-impact Issues

Missing checks for address(0x0) when assigning values to address state variables

1. File: contracts/core/PrePOMarket.sol (line 68)

       _treasury = _governance;

2. File: contracts/core/PrePOMarket.sol (line 176)

       _treasury = _newTreasury;

3. File: contracts/core/SingleStrategyController.sol (line 75)

       _vault = _newVault;

4. File: contracts/core/WithdrawHook.sol (line 30)

       _vault = _newVault;

5. File: contracts/core/DepositHook.sol (line 40)

       _vault = _newVault;

6. File: contracts/core/Collateral.sol (line 49)

       _treasury = _newTreasury;

abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

1. File: contracts/core/PrePOMarketFactory.sol (line 61)

       bytes32 _salt = keccak256(abi.encodePacked(_longToken, _shortToken));

2. File: contracts/core/AccountAccessController.sol (line 66)

       bytes32 _leaf = keccak256(abi.encodePacked(msg.sender));

Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.

1. File: contracts/core/PrePOMarketFactory.sol (lines 12-15)

   contract PrePOMarketFactory is
   IPrePOMarketFactory,
   OwnableUpgradeable,
   ReentrancyGuardUpgradeable

2. File: contracts/core/Collateral.sol (lines 13-17)

   contract Collateral is
   ICollateral,
   ERC20Upgradeable,
   OwnableUpgradeable,
   ReentrancyGuardUpgradeable

Non-critical Issues

Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate

1. File: contracts/core/CollateralDepositRecord.sol (lines 11-12)

   mapping(address => uint256) private _accountToNetDeposit;
   mapping(address => bool) private _allowedHooks;

Large multiples of ten should use scientific notation (e.g. 1e6) rather than decimal literals (e.g. 1000000), for readability

1. File: contracts/core/PrePOMarket.sol (line 32)

   uint256 private constant FEE_DENOMINATOR = 1000000;

2. File: contracts/core/Collateral.sol (line 35)

   uint256 private constant FEE_DENOMINATOR = 1000000;

Use a more recent version of solidity

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)

1. File: contracts/core/PrePOMarketFactory.sol (line 2)

   pragma solidity =0.8.7;

2. File: contracts/core/AccountAccessController.sol (line 2)

   pragma solidity =0.8.7;

Non-exploitable reentrancies

Follow the best-practice of the Checks-Effects-Interactions pattern Reentrancy in Collateral.deposit(uint256) (contracts/core/Collateral.sol#52-93): External calls:

• _baseToken.safeTransferFrom(msg.sender,address(this),_amount) (contracts/core/Collateral.sol#59)
• _depositHook.hook(msg.sender,_amount,_amountToDeposit) (contracts/core/Collateral.sol#64)
• _baseToken.safeTransfer(_treasury,_fee) (contracts/core/Collateral.sol#72)
• _baseToken.approve(address(_strategyController),_amountToDeposit) (contracts/core/Collateral.sol#76)
• _strategyController.deposit(_amountToDeposit) (contracts/core/Collateral.sol#77) State variables written after the call(s):
• _mint(msg.sender,_shares) (contracts/core/Collateral.sol#91)
  • _balances[account] += amount (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol#264)
• _mint(msg.sender,_shares) (contracts/core/Collateral.sol#91)
  • _totalSupply += amount (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol#263)

Reentrancy in PrePOMarketFactory.createMarket(string,string,address,address,uint256,uint256,uint256,uint256,uint256,uint256,uint256) (contracts/core/PrePOMarketFactory.sol#42-82): External calls:

• _longToken.transferOwnership(address(_newMarket)) (contracts/core/PrePOMarketFactory.sol#79)
• _shortToken.transferOwnership(address(_newMarket)) (contracts/core/PrePOMarketFactory.sol#80) Event emitted after the call(s):
• MarketAdded(address(_newMarket),_salt) (contracts/core/PrePOMarketFactory.sol#81) Reentrancy in Collateral.deposit(uint256) (contracts/core/Collateral.sol#52-93): External calls:
• _baseToken.safeTransferFrom(msg.sender,address(this),_amount) (contracts/core/Collateral.sol#59)
• _depositHook.hook(msg.sender,_amount,_amountToDeposit) (contracts/core/Collateral.sol#64)
• _baseToken.safeTransfer(_treasury,_fee) (contracts/core/Collateral.sol#72)
• _baseToken.approve(address(_strategyController),_amountToDeposit) (contracts/core/Collateral.sol#76)
• _strategyController.deposit(_amountToDeposit) (contracts/core/Collateral.sol#77) Event emitted after the call(s):
• Transfer(address(0),account,amount) (node_modules/@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol#265)
  • _mint(msg.sender,_shares) (contracts/core/Collateral.sol#91) Reentrancy in SingleStrategyController.migrate(IStrategy) (contracts/core/SingleStrategyController.sol#51-72): External calls:
• _baseToken.approve(address(_newStrategy),type()(uint256).max) (contracts/core/SingleStrategyController.sol#60)
• _baseToken.approve(address(_oldStrategy),0) (contracts/core/SingleStrategyController.sol#62)
• _oldStrategy.withdraw(address(this),_oldStrategyBalance) (contracts/core/SingleStrategyController.sol#64)
• _newStrategy.deposit(_baseToken.balanceOf(address(this))) (contracts/core/SingleStrategyController.sol#65) Event emitted after the call(s):
• StrategyMigrated(address(_oldStrategy),address(_newStrategy),_oldStrategyBalance) (contracts/core/SingleStrategyController.sol#67-71) Reentrancy in PrePOMarket.mintLongShortTokens(uint256) (contracts/core/PrePOMarket.sol#101-128): External calls:
• _collateral.transferFrom(msg.sender,_treasury,_fee) (contracts/core/PrePOMarket.sol#121)
• _collateral.transferFrom(msg.sender,address(this),_amount) (contracts/core/PrePOMarket.sol#123)
• _longToken.mint(msg.sender,_amount) (contracts/core/PrePOMarket.sol#124)
• _shortToken.mint(msg.sender,_amount) (contracts/core/PrePOMarket.sol#125) Event emitted after the call(s):
• Mint(msg.sender,_amount) (contracts/core/PrePOMarket.sol#126) Reentrancy in PrePOMarket.redeem(uint256,uint256) (contracts/core/PrePOMarket.sol#130-173): External calls:
• _longToken.burnFrom(msg.sender,_longAmount) (contracts/core/PrePOMarket.sol#158)
• _shortToken.burnFrom(msg.sender,_shortAmount) (contracts/core/PrePOMarket.sol#159)
• _collateral.transfer(_treasury,_fee) (contracts/core/PrePOMarket.sol#168)
• _collateral.transfer(msg.sender,_collateralOwed) (contracts/core/PrePOMarket.sol#170) Event emitted after the call(s):
• Redemption(msg.sender,_collateralOwed) (contracts/core/PrePOMarket.sol#172)

[ramenforbreakfast]

Low-impact Missing zero address checks is duplicate of #35 abi.encodePacked() is a valid suggestion upgradeable contract gap would add additional complexity, we did not explicitly design these contracts to be inherited from.

Non-critical Multiple address mappings is not valid, the address keys for each mapping refer to different values. Large multiples of ten this is a style choice (severity 0) that we will consider Use a more recent version of solidity - duplicate of #103 Non-exploitable reentrancies - I do not think this is worth considering. We also already use nonReentrant for this purpose.

I will maintain the severity of this issue due to the valid abi.encodePacked suggestion.