prePO contest - kirk-baird's results

Lines of code

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

Vulnerability details

Impact

If a strategy does not have sufficient funds to withdraw() for the full amount then it is possible that tokens will be left in this yield contract during migrate().

It is common for withdrawal from a strategy to withdraw less than a user's balance. The reason is that these yield protocols may lend the deposited funds to borrowers, if there is less funds in the pool than the withdrawal amount the withdrawal may succeed but only transfer the funds available rather than the full withdrawal amount.

The impact of tokens remaining in the old strategy is that when we call StrategyController.totalValue() this will only account for the tokens deposited in the new strategy and not those stuck in the previous strategy. Therefore totalValue() is undervalued.

Thus, when a user calls Collateral.deposit() the share calculations _shares = (_amountToDeposit * totalSupply()) / (_valueBefore); will be over stated (note: uint256 _valueBefore = _strategyController.totalValue();). Hence, the user will receive more shares than they should.

The old tokens may be recovered by calling migrate() back to the old strategy. If this is done then totalValue() will now include the tokens previously stuck. The recent depositer may now withdraw and will be owed (_strategyController.totalValue() * _amount) / totalSupply(). Since totalValue() is now includes the previously stuck tokens _owed will be overstated and the user will receive more collateral than they should.

The remaining users who had deposited before migrate() will lose tokens proportional to their share of the totalSupply().

Proof of Concept

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

    function migrate(IStrategy _newStrategy)
        external
        override
        onlyOwner
        nonReentrant
    {
        uint256 _oldStrategyBalance;
        IStrategy _oldStrategy = _strategy;
        _strategy = _newStrategy;
        _baseToken.approve(address(_newStrategy), type(uint256).max);
        if (address(_oldStrategy) != address(0)) {
            _baseToken.approve(address(_oldStrategy), 0);
            _oldStrategyBalance = _oldStrategy.totalValue();
            _oldStrategy.withdraw(address(this), _oldStrategyBalance);
            _newStrategy.deposit(_baseToken.balanceOf(address(this)));
        }
        emit StrategyMigrated(
            address(_oldStrategy),
            address(_newStrategy),
            _oldStrategyBalance
        );
    }

Recommended Mitigation Steps

The recommendation is to ensure that require(_oldStrategy.totalValue() == 0) after calling _oldStrategy.withdraw(). This ensures that no funds are left in the strategy. Consider the code example below.

    function migrate(IStrategy _newStrategy)
        external
        override
        onlyOwner
        nonReentrant
    {
        uint256 _oldStrategyBalance;
        IStrategy _oldStrategy = _strategy;
        _strategy = _newStrategy;
        _baseToken.approve(address(_newStrategy), type(uint256).max);
        if (address(_oldStrategy) != address(0)) {
            _baseToken.approve(address(_oldStrategy), 0);
            _oldStrategyBalance = _oldStrategy.totalValue();
            _oldStrategy.withdraw(address(this), _oldStrategyBalance);
            require(_oldStrategy.totalValue() == 0)
            _newStrategy.deposit(_baseToken.balanceOf(address(this)));
        }
        emit StrategyMigrated(
            address(_oldStrategy),
            address(_newStrategy),
            _oldStrategyBalance
        );
    }

Lines of code

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

Vulnerability details

Impact

The is a potential rug pull vector for a user who manipulates the _governance parameter in PrePOMarketFactory::createMarket(). A user who manipulates this value to an address they own will become the owner of the PrePOMarket.sol.

The owner of PrePOMarket.sol may rug pull the protocol by following these steps:

1. Mint numerous tokens in mintLongShortTokens()
2. Use the AMM to purchase all LONG tokens (or SHORT)
3. Call the onlyOwner function setFinalLongPrice() to set the value to _ceilingLongPrice (or _floorLongPrice)
4. Call redeem() to gain the collateral tokens
5. Withdraw the collateral tokens for the base tokens

Steps 1-4 can be done in a single transaction allowing the attacker to pull off this attack without allowing time for the DAO to prevent it.

Proof of Concept

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

    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 {
        require(_validCollateral[_collateral], "Invalid collateral");

        (
            LongShortToken _longToken,
            LongShortToken _shortToken
        ) = _createPairTokens(_tokenNameSuffix, _tokenSymbolSuffix);
        bytes32 _salt = keccak256(abi.encodePacked(_longToken, _shortToken));

        PrePOMarket _newMarket = new PrePOMarket{salt: _salt}(
            _governance,
            _collateral,
            ILongShortToken(address(_longToken)),
            ILongShortToken(address(_shortToken)),
            _floorLongPrice,
            _ceilingLongPrice,
            _floorValuation,
            _ceilingValuation,
            _mintingFee,
            _redemptionFee,
            _expiryTime,
            false
        );
        _deployedMarkets[_salt] = address(_newMarket);

        _longToken.transferOwnership(address(_newMarket));
        _shortToken.transferOwnership(address(_newMarket));
        emit MarketAdded(address(_newMarket), _salt);
    }

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

    constructor(
        address _governance,
        address _newCollateral,
        ILongShortToken _newLongToken,
        ILongShortToken _newShortToken,
        uint256 _newFloorLongPrice,
        uint256 _newCeilingLongPrice,
        uint256 _newFloorValuation,
        uint256 _newCeilingValuation,
        uint256 _newMintingFee,
        uint256 _newRedemptionFee,
        uint256 _newExpiryTime,
        bool _allowed
    ) {
        require(
            _newCeilingLongPrice > _newFloorLongPrice,
            "Ceiling must exceed floor"
        );
        require(_newExpiryTime > block.timestamp, "Invalid expiry");
        require(_newMintingFee <= FEE_LIMIT, "Exceeds fee limit");
        require(_newRedemptionFee <= FEE_LIMIT, "Exceeds fee limit");
        require(_newCeilingLongPrice <= MAX_PRICE, "Ceiling cannot exceed 1");

        transferOwnership(_governance);
        _treasury = _governance;
...

Recommended Mitigation Steps

Consider enforcing _governance to be set to the owner of PrePOMarketFactory.sol, which is the governance contract, as seen in the code below.

    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 {
        require(_validCollateral[_collateral], "Invalid collateral");

        (
            LongShortToken _longToken,
            LongShortToken _shortToken
        ) = _createPairTokens(_tokenNameSuffix, _tokenSymbolSuffix);
        bytes32 _salt = keccak256(abi.encodePacked(_longToken, _shortToken));

        PrePOMarket _newMarket = new PrePOMarket{salt: _salt}(
            owner(),
            _collateral,
            ILongShortToken(address(_longToken)),
            ILongShortToken(address(_shortToken)),
            _floorLongPrice,
            _ceilingLongPrice,
            _floorValuation,
            _ceilingValuation,
            _mintingFee,
            _redemptionFee,
            _expiryTime,
            false
        );
        _deployedMarkets[_salt] = address(_newMarket);

        _longToken.transferOwnership(address(_newMarket));
        _shortToken.transferOwnership(address(_newMarket));
        emit MarketAdded(address(_newMarket), _salt);
    }