QA Report

[code423n4]

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

PrePOMarket.FEE_DENOMINATOR: 1000000 -> use 1e6

---

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 {

[ramenforbreakfast]

Non-Critical
Contract implementations should inherit their interface - is a valid consideration of severity 0
Use scientific notation 1e10 instead of using many zeros - is an incorrect version of a duplicate suggestion.
Boolean constants can be used directly and do not need to be compare to true or false - duplicate of #5
Remove empty blocks of code - duplicate of #5
Use interface ICollateral instead of IERC20 - is a valid consideration of severity 0

Low-Risk
Wrong calculation of shares mentioned in comment of Collateral.deposit() - duplicate of #40
Zero-address checks are missing - duplicate of #35
IPrePOMarketFactory wrong interface - duplicate of #30

Since the only suggestions that are not duplicates are of severity 0, will request this as needing a lower severity.