QA Report

[code423n4]

Codebase Impressions & Summary

Overall, code quality for the PrePO contracts is very high. The contracts are well modularised and are clear enough to follow. The provided documentation was adequate in explaining key concepts like the pricing formulas for the long and short tokens, and valuation ranges. Special mention for providing a video walkthrough as well!

The contracts have 100% coverage, which unfortunately isn’t the norm. Kudos for having adequate tests to fully test the core contracts!

In total, there were 1 high, 2 medium and 5 low findings reported. The high severity issue pertains to a vault related edge case that describes a scenario where a malicious actor is able to DOS other users by artificially inflating the value of a single unit of collateral token. That aside, the quantity of other findings appropriately reflect the quality of the source code as it conforms closely to smart contract best practices.

Low Severity Findings

L01: Ensure _ceilingValuation > _floorValuation

Line References

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/PrePOMarket.sol#L78-L79

Description

No check was performed that the _ceilingValuation exceeds _floorValuation. While it bears no impact to functionality, it would be ideal to ensure that the valuations are correctly set.

Recommended Mitigation Steps

require(_newCeilingValuation > _newFloorValuation, "Ceiling must exceed floor");

L02: PrePOMarketFactory’s createMarket() has different parameter order in interface and contract

Line References

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/PrePOMarketFactory.sol#L45-L46

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/interfaces/IPrePOMarketFactory.sol#L52-L53

Description

The _governance and _collateral parameters are swapped in the interface and implementation. There is thankfully a check to ensure that the collateral is whitelisted before the market can be created (so market creation would have reverted). Otherwise, the variables would have been set incorrectly.

Recommended Mitigation Steps

Swap _governance and _collateral parameters in the interface.

L03: Duplicate shortToken param natspec in IPrePOMarket

Line References

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/interfaces/IPrePOMarket.sol#L18-L19

Description

shortToken parameter is duplicated.

Recommended Mitigation Steps

Remove either instance.

L04: _delayedWithdrawalExpiry adds unnecessary complexity and potentially griefs users

Line References

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/Collateral.sol#L124-L127

Description

The main motivation for _delayedWithdrawalExpiry is to mitigate flash loan attacks. In our opinion, having a validity period for withdrawals provides negligible protection.

The _delayedWithdrawalExpiry variable potentially griefs a majority of users if it is set a low value (1 block for example), where it becomes difficult for the average user to perform withdrawals.

Recommended Mitigation Steps

Remove _delayedWithdrawalExpiry and its corresponding check. A better way to mitigate flash loans is to prevent users from depositing and withdrawing in the same block.

L05: Set _governance and _treasury in factory

Line References

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/PrePOMarketFactory.sol#L45-L46

Description

The governance and treasury addresses are passed in as parameters whenever a market is created. It would be safer to save and retrieve these addresses to / from storage to avoid input errors.

Note that we are aware that it is mentioned in the README that gas optimization will not be awarded for struct packing but this low issue pertains to having a canonical source to retrieve the _governance and _treasury address (within the factory) to make it impossible for the owner to accidentally use the wrong address.

Recommended Mitigation Steps

To counter the dreaded stack too deep problem, we pack the parameters into a struct. This avoids the need to first initialize the treasury address as the governance address as well.

// PrePOMarketFactory.sol
// TODO: create getters and mutators for governance and treasury variables
address private _governance;
address private _treasury;

// notice the removal of _governance and _treasury
// TODO: Modify overriden interface function
function createMarket(
  string memory _tokenNameSuffix,
  string memory _tokenSymbolSuffix,
  address _collateral,
  uint256 _floorLongPrice,
  uint256 _ceilingLongPrice,
  uint256 _floorValuation,
  uint256 _ceilingValuation,
  uint256 _mintingFee,
  uint256 _redemptionFee,
  uint256 _expiryTime
) external override onlyOwner nonReentrant {
	...
	PrePOMarket _newMarket = new PrePOMarket{salt: _salt}(
    PrePOMarket.MarketInitParams({
	    governance: _governance,
	    treasury: _treasury,
	    collateral: _collateral,
	    longToken: ILongShortToken(address(_longToken)),
	    shortToken: ILongShortToken(address(_shortToken)),
	    floorLongPrice: _floorLongPrice,
	    ceilingLongPrice: _ceilingLongPrice,
	    floorValuation: _floorValuation,
	    ceilingValuation: _ceilingValuation,
	    mintingFee: _mintingFee,
	    redemptionFee: _redemptionFee,
	    expiryTime: _expiryTime
    })
  );
	...
}

// PrePOMarket.sol
struct MarketInitParams {
  address governance;
  address treasury;
  address collateral;
  ILongShortToken longToken;
  ILongShortToken shortToken;
  uint256 floorLongPrice;
  uint256 ceilingLongPrice;
  uint256 floorValuation;
  uint256 ceilingValuation;
  uint256 mintingFee;
  uint256 redemptionFee;
  uint256 expiryTime;
}

/**
 * Assumes `_newCollateral`, `_newLongToken`, and `_newShortToken` are
 * valid, since they will be handled by the PrePOMarketFactory.
 *
 * Assumes that ownership of `_longToken` and `_shortToken` has been
 * transferred to this contract via `createMarket()` in
 * `PrePOMarketFactory.sol`.
 */
constructor(MarketInitParams memory marketParams) {
	require(
	    marketParams.ceilingLongPrice > marketParams.floorLongPrice,
	    "Ceiling must exceed floor"
	);
	require(marketParams.expiryTime > block.timestamp, "Invalid expiry");
	require(marketParams.mintingFee <= FEE_LIMIT, "Exceeds fee limit");
	require(marketParams.redemptionFee <= FEE_LIMIT, "Exceeds fee limit");
	require(marketParams.ceilingLongPrice <= MAX_PRICE, "Ceiling cannot exceed 1");
	
	transferOwnership(marketParams.governance);
	_treasury = marketParams.treasury;
	
	_collateral = IERC20(marketParams.collateral);
	_longToken = marketParams.longToken;
	_shortToken = marketParams.shortToken;
	
	_floorLongPrice = marketParams.floorLongPrice;
	_ceilingLongPrice = marketParams.ceilingLongPrice;
	_finalLongPrice = MAX_PRICE + 1;
	
	_floorValuation = marketParams.floorValuation;
	_ceilingValuation = marketParams.ceilingValuation;
	
	_mintingFee = marketParams.mintingFee;
	_redemptionFee = marketParams.redemptionFee;
	
	_expiryTime = marketParams.expiryTime;
	
	emit MarketCreated(
	  address(marketParams.longToken),
	  address(marketParams.shortToken),
	  marketParams.floorLongPrice,
	  marketParams.ceilingLongPrice,
	  marketParams.floorValuation,
	  marketParams.ceilingValuation,
	  marketParams.mintingFee,
	  marketParams.redemptionFee,
	  marketParams.expiryTime
	);
}

Suggestions

S01: Make Collateral contract EIP4626 Compliant

Description

The EIP4626 standard has just been finalised. We recommend making the Collateral contract compliant to the standard. As our favourite optimisoooor @t11s succinctly puts it, “there are a terrifying amount of pitfalls that you can run into when writing a vault from scratch, i learned this first hand working on them at rari. If you want to sleep at night, skip that bs and use 4626”

References

Twitter thread by @t11s shilling EIP4626

Solmate’s base implementation

OpenZeppelin’s draft EIP

[ramenforbreakfast]

L01 i think is unnecessary and adds additional complexity for something that is not actually used within contract logic.
L02-03 are valid submissions and good catches!
L04 We added expiries as an intentional decision for protection, so will not be considering this as valid.
L05 Governance and treasury were designed so that they can be different for different markets and thus this is not an issue.

Keeping this severity since L02-03 are valid suggestions. Nicely organized report!