prePO contest - berndartmueller's results

QA Report

Non-Critical Findings

Contract implementations should inherit their interface

Description

It's best practice for a contract to inherit from it's interface. This improves the contract's clarity and makes sure the contract implementation complies with the defined interface.

Findings

LongShortToken.sol:

contract LongShortToken is ILongShortToken, ERC20Burnable, Ownable { // @audit-info add interface ILongShortToken
  ...
}

Recommended mitigation steps

Inherit from the missing interface or contract.

---

Use scientific notation 1e10 instead of using many zeros

Description

For better readability and to prevent any issues, use the scientific notation 1e10 instead of e.g. 1000000

Findings

Collateral.FEE_DENOMINATOR: 1000000 -> use 1e6<br/> PrePOMarket.FEE_DENOMINATOR: 1000000 -> use 1e6<br/>

---

Boolean constants can be used directly and do not need to be compare to true or false

Description

Boolean constants can be used directly and do not need to be compare to true or false.

Findings

Collateral.deposit()

Recommended mitigation steps

Remove the equality to the boolean constant.

---

Remove empty blocks of code

Description

Code contains empty block. See https://protofire.github.io/solhint/docs/rules/best-practises/no-empty-blocks.html

Findings

AccountAccessController.constructor()

Recommended mitigation steps

Remove empty code block

---

Use interface ICollateral instead of IERC20

Description

Use more specific interface ICollateral instead of the general interface IERC20 for code clarity.

Findings

PrePOMarket._collateral PrePOMarket.constructor()

Recommended mitigation steps

Change variable type IERC20 to ICollateral:

ICollateral private immutable _collateral;

and

_collateral = ICollateral(_newCollateral);

Low Risk

Wrong calculation of shares mentioned in comment of Collateral.deposit()

Description

In the comments, right next to the actual implementation of the calculation of shares, the formula is wrong. The implementation itself is correct.

Wrong calculation:

/**
  * # of shares owed = amount deposited / cost per share, cost per
  * share = total supply / total value.
  */

Correct calculation:

/**
  * # of shares owed = amount deposited / cost per share, cost per
  * share = total value / total supply. @audit-info swapped total value with total supply
  */

Findings

Collateral.deposit()

Recommended mitigation steps

Fix comment to prevent confusion with actual implementation.

---

Zero-address checks are missing

Description

Zero-address checks are a best-practice for input validation of critical address parameters. While the codebase applies this to most most cases, there are many places where this is missing in constructors and setters.

Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens or force redeployment of contracts.

Findings

Collateral.initialize()#_newTreasury

Recommended mitigation steps

Add zero-address checks, e.g.:

require(address(_newTreasury) != address(0), "Zero address");

---

Parameter order of PrePOMarketFactory.createMarket() is different than defined in interface IPrePOMarketFactory.createMarket()

Description

The order of parameters in the contract implementation PrePOMarketFactory is different than how parameters are defined in the interface.

Findings

PrePOMarketFactory.createMarket()

Recommended mitigation steps

Either change the order of parameters in the interface IPrePOMarketFactory to match the implementation or update the implementation.

IPrePOMarketFactory.createMarket()

function createMarket(
    string memory _tokenNameSuffix,
    string memory _tokenSymbolSuffix,
    address _governance,
    address _collateral,
    uint256 _floorLongPrice,
    uint256 _ceilingLongPrice,
    uint256 _floorValuation,
    uint256 _ceilingValuation,
    uint256 _mintingFee,
    uint256 _redemptionFee,
    uint256 _expiryTime
) external override onlyOwner nonReentrant {

PrePOMarketFactory.createMarket()

function createMarket(
    string memory tokenNameSuffix,
    string memory tokenSymbolSuffix,
    address collateral, // @audit-info `collateral` and `governance` parameters are switched compared to how they are defined in the interface
    address governance,
    uint256 floorLongPrice,
    uint256 ceilingLongPrice,
    uint256 floorValuation,
    uint256 ceilingValuation,
    uint256 mintingFee,
    uint256 redemptionFee,
    uint256 expiryTime
) external override onlyOwner nonReentrant {

Gas Optimizations

Removing unused function parameter can save gas

Description

Removing unused function parameters can save gas. And it improves code clarity.

Findings

IHook.hook()
DepositHook.hook()
WithdrawHook.hook()

Recommended mitigation steps

Remove unused function parameter initialAmount.

---

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.

Findings

AccountAccessController.allowAccounts()
AccountAccessController.blockAccounts()

Recommended mitigation steps

AccountAccessController.allowAccounts()

uint length = _accounts.length;

for (uint256 _i = 0; _i < length; _i++) {
    _allowedAccounts[_allowedAccountsIndex][_accounts[_i]] = true;
    emit AccountAllowed(_accounts[_i]);
}

AccountAccessController.blockAccounts()

uint length = _accounts.length;

for (uint256 _i = 0; _i < length; _i++) {
    _blockedAccounts[_blockedAccountsIndex][_accounts[_i]] = true;
    emit AccountBlocked(_accounts[_i]);
}

---

Use a temporary variable to cache repetitive storage reads

Reading the same variable from storage multiple times consumes unnecessary gas because SLOADs are expensive.

Findings

AccountAccessController.allowAccounts()

Repetitive storage access of variable: _accounts[_i]

1st access - L45
2nd access - L46

AccountAccessController.blockAccounts()

Repetitive storage access of variable: _accounts[_i]

1st access - L56
2nd access - L57

Recommended mitigation steps

Read the variable from storage once and store it in a temporary variable in memory for repetitive read access.

---

Unused named returns can be removed

Description

Removing unused named return variables can reduce gas usage and improve code clarity.

Recommended mitigation steps

Remove the unused named return variables or use them instead of creating additional variables.

Findings

PrePOMarketFactory.sol

L98 unnecessary named returns _newLongToken & _newShortToken