prePO contest - rayn's results

Lines of code

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

Vulnerability details

Impact

This bug enables partial bypass of fee while minting Collateral tokens through Collateral.deposit. Attackers can also utilize this bug to inflate prices of Collateral tokens, creating "unfair advantages" for early minters of Collateral tokens.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

In Collateral.deposit, the amount to deposit is calculated several times.

1. The passed in _amount is used to fetch how much to transfer from msg.sender to Collateral

_baseToken.safeTransferFrom(msg.sender, address(this), _amount);

2. Then, balance in _baseToken of Collateral is fetched for fee calculation

        uint256 _amountToDeposit = _baseToken.balanceOf(address(this));
        ...
        uint256 _fee = (_amountToDeposit * _mintingFee) / FEE_DENOMINATOR + 1;
        require(_amountToDeposit > _fee, "Deposit amount too small");
        _baseToken.safeTransfer(_treasury, _fee);
        _amountToDeposit -= _fee;

3. Finally, difference in _strategyController.totalValue() is used to recalculate _amountToDeposit. Apart from this, _strategyController deposits _baseToken.balanceOf(address(this)) to _stategy instead of using _amount passed to it

Collateral
    function deposit(uint256 _amount)
        external
        override
        nonReentrant
        returns (uint256)
    {
        ...
        uint256 _valueBefore = _strategyController.totalValue();
        _baseToken.approve(address(_strategyController), _amountToDeposit);
        _strategyController.deposit(_amountToDeposit);
        uint256 _valueAfter = _strategyController.totalValue();
        _amountToDeposit = _valueAfter - _valueBefore;
    }

SingleStrategyController
    function deposit(uint256 _amount)
        external
        override
        onlyVault
        nonReentrant
    {
        _baseToken.safeTransferFrom(_vault, address(this), _amount);
        _strategy.deposit(_baseToken.balanceOf(address(this)));
    }

    function totalValue() external view override returns (uint256) {
        return _baseToken.balanceOf(address(this)) + _strategy.totalValue();
    }

The problem is _baseToken.balanceOf(address(this)) in _strategyController.deposit is susceptible to manipulation. An attacker can simply transfer some _baseToken to _strategyController directly before calling Collateral.deposit to make the passed _amount and _baseToken.balanceOf(address(this)) mismatch

However, while the attacker is able to deposit more _baseToken to _strategy than expected, the calculated _shares does not increase since it relies on _amountToDeposit calculated from _strategyController.totalValue(), which is not affected by the manipulation of _baseToken balance of _strategyController

        uint256 _shares = 0;
        if (totalSupply() == 0) {
            _shares = _amountToDeposit;
        } else {
            /**
             * # of shares owed = amount deposited / cost per share, cost per
             * share = total supply / total value.
             */
            _shares = (_amountToDeposit * totalSupply()) / (_valueBefore);
        }
        _mint(msg.sender, _shares);

The immediate consequence of increased _baseToken deposited and unaffected _shares is that less Collateral tokens are minted than expected, leading to the inflation of the Collateral tokens, and create an "unfair" advantage for minters before inflation to those that joined after inflations.

To sum up the attack, the attacker creates a contract to do those actions listed below in a single transaction

1. _baseToken.deposit(AMOUNT1+AMOUNT2+AMOUNT3)
2. Collateral.deposit(AMOUNT1)
3. _baseToken.transfer(_strategyController, AMOUNT2)
4. Collateral.deposit(AMOUNT3) <- this triggers inflation
5. profit

The effect of the attack is

1. The attacker enjoys inflated value of AMOUNT1 Collateral tokens minted before inflation
2. The attacker pays minting fee for AMOUNT1+AMOUNT3 while depositing an additional unaccounted AMOUNT2 _baseTokens into _strategy

Tools Used

None

Recommended Mitigation Steps

Since it is reasonable to disallow users from transferring _baseToken to _strategyController directly, an easy remedy is to change _strategyController to deposit _amount to _strategy

SingleStrategyController
    function deposit(uint256 _amount)
        external
        override
        onlyVault
        nonReentrant
    {
        _baseToken.safeTransferFrom(_vault, address(this), _amount);
        _strategy.deposit(_amount);
    }

To prevent the token lockup if anyone accidentally transfers to _strategyController, it is possible to utilize that _strategyController should never own any token except during Collateral.deposit, and add a skim function to _strategyController to allow anyone to remove excessive tokens in the contract.

SingleStrategyController
    function skim(address to)
        external
        override
        onlyVault
        nonReentrant
    {
        _baseToken.safeTransferFrom(to, _baseToken.balanceOf(address(this)));
    }

Low Risk Findings - expiry is not work in contract

In document, we define that:

### Expiry

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

https://docs.prepo.io/concepts/markets#expiry

But in contract, the variable _expiryTime doesn't settle at the lower bound of its Valuation Range. Somebody can still mint LongShortTokens if market expired and owner doesn't set _floorLongPrice.

Recommendation

Check _expiryTime in mintLongShortTokens:

if (block.timestamp > _expiryTime && _finalLongPrice == MAX_PRICE + 1) {
    _finalLongPrice = _floorLongPrice;
}

---

Non-Critical Findings - Unused function parameter

In DepositHook.sol and WithdrawHook.sol: https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/DepositHook.sol#L28 https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/WithdrawHook.sol#L23

The parameter _initialAmount is unused.

Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
  --> contracts/core/DepositHook.sol:28:9:
   |
28 |         uint256 _initialAmount,
   |         ^^^^^^^^^^^^^^^^^^^^^^


Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
  --> contracts/core/WithdrawHook.sol:23:9:
   |
23 |         uint256 _initialAmount,
   |

Recommendation

Delete _initialAmount parameter.

---

Non-Critical Findings - Uninitialized parent contract

In initialize() of PrePOMarketFactory.sol: https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/PrePOMarketFactory.sol#L20

The contract PrePOMarketFactory inherit ReentrancyGuardUpgradeable but not initialize it.

Recommendation

Change to:

  20     function initialize() public initializer {
  21         OwnableUpgradeable.__Ownable_init();
+ 22         ReentrancyGuardUpgradeable.__ReentrancyGuard_init();
  23     }

---

Non-Critical Findings - Redundant code for return value

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

It's unnecessary to return value in L117 due to provides names for variables in L98: https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/PrePOMarketFactory.sol#L98

Recommendation

Delete L117.

---

Non-Critical Findings - allowSelf doesn't revert when blocked account call this function

In the definition of allowSelf in IAccountAccessController.sol:

    /**
     * @notice Allows the caller if the provided signature is valid.
     * @dev An account cannot call this function if it is already
     * allowed/blocked.
     * @param proof Proof of the caller's inclusion in the merkle root
     */
    function allowSelf(bytes32[] calldata proof) external;

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

An account cannot call this function if it is already allowed/blocked, but a blocked account can still call this function.

Recommendation

Check the account is blocked in allowSelf function: https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/AccountAccessController.sol#L61

     function allowSelf(bytes32[] calldata _proof) external override {
         require(
             _allowedAccounts[_allowedAccountsIndex][msg.sender] == false,
             "Account already registered"
         );
+        require(
+            _blockedAccounts[_blockedAccountsIndex][msg.sender] == false,
+            "Account already blocked"

Save gas in for loops by unchecked arithmetic

The for loops in allowAccounts and blockAccounts: https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/AccountAccessController.sol#L44-L47 https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/AccountAccessController.sol#L55-L58

There is no overflow risk of _i.

Recommendation

Use unchecked block to avoid overflow checks:

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

Send _baseToken in deposit function directly to save gas

Currently the Collateral contract approve _strategyController to transfer the base token: https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/Collateral.sol#L76 https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/SingleStrategyController.sol#L38

We can send base token to _strategyController directly to save gas.

Recommendation

Change in Collateral.sol:

@@ -73,7 +73,10 @@ contract Collateral
         _amountToDeposit -= _fee;

         uint256 _valueBefore = _strategyController.totalValue();
-        _baseToken.approve(address(_strategyController), _amountToDeposit);
+        _baseToken.safeTransfer(
+            address(_strategyController),
+            _amountToDeposit
+        );
         _strategyController.deposit(_amountToDeposit);
         uint256 _valueAfter = _strategyController.totalValue();
         _amountToDeposit = _valueAfter - _valueBefore;

Change in SingleStrategyController.sol:

@@ -35,7 +35,6 @@ contract SingleStrategyController is
         onlyVault
         nonReentrant
     {
-        _baseToken.safeTransferFrom(_vault, address(this), _amount);
         _strategy.deposit(_baseToken.balanceOf(address(this)));
     }