prePO contest - hake's results

Total Findings (including sub-topics): X

• I attempted to group some findings under the same umbrella to ease readability/reporting.

Finding 1.0

• Checking/Setting Addresses

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

In DepositHook.sol:

constructor(address _newAccessController, address _newDepositRecord) {
        _accountAccessController = IAccountAccessController( _newAccessController);
        _depositRecord = ICollateralDepositRecord(_newDepositRecord);
    }

There are no zero address checks in the constructor and no functions that allow for setting the addresses of _accountAccessController and _depositRecord. Therefore, if the wrong addresses were used, a whole new contract would have to be deployed to resolve the issue.

(Exact same finding at WithdrawHook.sol regarding _depositRecord)

Recommendation 1

Implement require() functions in the constructor.

constructor(address _newAccessController, address _newDepositRecord) {
        require(_newAccessController != address(0), "Zero address for AccessController");
        require( _newDepositRecord != address(0), "Zero address for DepositRecord");
        _accountAccessController = IAccountAccessController(_newAccessController);
        _depositRecord = ICollateralDepositRecord(_newDepositRecord);

and/or

Implement a guarded setter function to change the parameters

    _accountAccessController = IAccountAccessController(_newAccessController);

    function setDepositRecord(address _newDepositRecord)external onlyOwner {
    _depositRecord = ICollateralDepositRecord(_newDepositRecord);
}

If implementing setter functions I also suggest implementing events in those.

Finding 1.1

No zero address check for setVault (https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/DepositHook.sol#L39-L42).

Finding 1.2

In Collatereal.sol (https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/Collateral.sol#L38-L50). No zero address check for _baseToken and _treasury and no way to set new addresses for neither.

Finding 1.3 (recommendation) (SingleStrategyController.sol)

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

Recommend to include the zero address check for _oldStrategy in the constructor and remove it from migrate() as the event omitted from a migration from address(0) would not contain meaningful information. On a similar note I recommed including a zero address check using require() in setVault().

Finding 2 (DepositHook.sol) - (CollateralDepositRecord.sol):

• Unused function Input

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/DepositHook.sol#L26-L37

function hook(
       address _sender,
       uint256 _initialAmount,
       uint256 _finalAmount
   ) external override onlyVault {
       require(
           _accountAccessController.isAccountAllowed(_sender) &&
               !_accountAccessController.isAccountBlocked(_sender),
           "Account not allowed to deposit"
       );
       _depositRecord.recordDeposit(_sender, _finalAmount);
   }

Input _initialAmount is never used. My interpretation is that it is meant to be used in: _depositRecord.recordDeposit(_sender,_finalAmount) https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/CollateralDepositRecord.sol#L24-L39

So instead of:

function recordDeposit(address _sender, uint256 _amount)
       external
       override
       onlyAllowedHooks
   {
       require(
           _amount + _globalDepositAmount <= _globalDepositCap,
           "Global deposit cap exceeded"
       );
       require(
           _amount + _accountToNetDeposit[_sender] <= _accountDepositCap,
           "Account deposit cap exceeded"
       );
       _globalDepositAmount += _amount;
       _accountToNetDeposit[_sender] += _amount;
   }

We should have something like:

function recordDeposit(address _sender,uint256 _initialAmount, uint256          _finalAmount)
       external
       override
       onlyAllowedHooks
   {
       require(
           _initialAmount + _globalDepositAmount <= _globalDepositCap,
           "Global deposit cap exceeded"
       );
       require(
           _finalAmount + _accountToNetDeposit[_sender] <= _accountDepositCap,
           "Account deposit cap exceeded"
       );
       _globalDepositAmount += _initialAmount;
       _accountToNetDeposit[_sender] += _finalAmount;
   }

If that is not the case please remove _initalAmount from the hook() function. Same things should be taken into consideration in hook() from WithdrawHook.sol, and recordWithdrawal() in CollateralDepositRecord.sol.

I also recommend including events for recordDeposit() and recordWithdrawal().

Finding 3 (DepositHook.sol) - (AccountAccessController.sol):

• AccessControl Misbehaviour

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/DepositHook.sol#L26-L37

function hook(
       address _sender,
       uint256 _initialAmount,
       uint256 _finalAmount
   ) external override onlyVault {
       require(
           _accountAccessController.isAccountAllowed(_sender) &&
               !_accountAccessController.isAccountBlocked(_sender),
           "Account not allowed to deposit"
       );
       _depositRecord.recordDeposit(_sender, _finalAmount);
   }

The two step require() function checking if isAccountAllowed could fail and an allowed account wouldnt go through if for example:

• Account is allowed (https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/AccountAccessController.sol#L39-L48)

• Account is blocked (https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/AccountAccessController.sol#L50-L59)

• Account is allowed again (assuming accounts havent been cleared).

• require() fails because when an account is either blocked or allowed the function responsible doesnt remove the account from its counterpart index. So it can be true for both.

Recommendation 3

Remove && !_accountAccessController.isAccountBlocked(_sender) from the require() function and remove accounts from the allowed index if they are being blocked.

function blockAccounts(address[] calldata _accounts)
        external
        override
        onlyOwner
    {
        for (uint256 _i = 0; _i < _accounts.length; _i++) {
            _blockedAccounts[_blockedAccountsIndex][_accounts[_i]] = true;
            _allowedAccounts[_allowedAccountsIndex][_accounts[_i]] = false;
            emit AccountBlocked(_accounts[_i]);
        }
    }

Finding 4 (Collateral.sol)

• Issues with deposit() and withdraw() in Collateral.sol.

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

Issue 1

Lines 62-65

// Record deposit before fee is taken
    if (address(_depositHook) != address(0)) {
        _depositHook.hook(msg.sender, _amount, _amountToDeposit);
    }

• Current zero address check for _depositHook has to be run with every deposit (not optimal for gas).
• If address is zero the rest of the function will still continue to run despite the lack of accounting because the if() statement wont revert. and the accessbility of accounts (allowed/blocked) wont be checked. So any blocked users will be able to deposit and withdraw assets freely, their transactions just wont be recorded by the accounting system. Same for withdraw().

Recommendation (issue 1)

I suggest the zero address check for _depositHook and withdrawHook should rather be implemented at deploying or setting stage or at the start of the function at line 57 with a require() function. That way if users are blocked they will never be able to deposit/withdraw and there wont be an accouting deficit.

Issue 2

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

Lines 73-79

_amountToDeposit -= _fee;

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

_amountToDeposit -= fee; seems to be equal to _amountToDeposit = _valueAfter - _valueBefore;. I believe the _valueAfter and _valueBefore calcualtions to be unnecessary.

Finding 5 (Collateral.sol)

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

• No systems in place to ensure parameters are set in the right order.

This might lead to a few problems. If by mistake _depositsAllowed and _withdrawalsAllowed are set to true before the _mintingFee and/or _redemptionFee are set, users can deposit and withdraw funds without paying any fees. Same is valid for totalSupply() that might enable early users or front-runners to get a disproportionate amount of shares. If same users were to take advatange of flashloans this could cause large amounts of shares/rewards being paid out without any fees being collected.

Recommendation 5

Set require() functions that ensure certain parameters are initialized before others. E.g

bool _mintingFeeDeployed;
bool _redemptionFeeDeployed;

function setMintingFee(uint256 _newMintingFee)
    external
    override
    onlyOwner
{
    require(_newMintingFee <= FEE_LIMIT, "Exceeds fee limit");
    _mintingFeeDeployed = true;
    _mintingFee = _newMintingFee;
    emit MintingFeeChanged(_mintingFee);
}

function setRedemptionFee(uint256 _newRedemptionFee)
        external
        override
        onlyOwner
    {
        require(_newRedemptionFee <= FEE_LIMIT, "Exceeds fee limit");
        _redemptionFeeDeployed = true;
        _redemptionFee = _newRedemptionFee;
        emit RedemptionFeeChanged(_redemptionFee);
    }

function setDepositsAllowed(bool _allowed) external override onlyOwner {
        require(_mintingFeeDeployed == true);
        require(_redemptionFeeDeployed == true);
        _depositsAllowed = _allowed;
        emit DepositsAllowedChanged(_allowed);
    }

    function setWithdrawalsAllowed(bool _allowed) external override onlyOwner {
        require(_mintingFeeDeployed == true);
        require(_redemptionFeeDeployed == true);
        _withdrawalsAllowed = _allowed;
        emit WithdrawalsAllowedChanged(_allowed);
    }

OR set these parameters at initialization and make sure there are require() statements in place to ensure they are not wrongly set.

I believe the likelyhood of these parameters being set in the wrong order is low as they should be executed with care, but never the less still a possibility.

Finding 6 (PrePOMarket.sol)

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

• No systems in place to ensure parameters are set in the right order (much like the examples given in Finding 5).

• Lack of zero address check for _governance and _collateral and no setter function for _collateral means that another contract would have to be deployed if inputs were wrong. I recommend implementing the zero address checks and/or setter functions.

Finding 7 (PrePOMarketFactory.sol)-(PrePOMarket.sol)

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

Constructor in PrePOMarket.sol has 12 parameters with the last one being bool _allowed.

When creating a _newMarket in PrePOMarketFactory.sol this last parameter is simply set to false https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/PrePOMarketFactory.sol#L63-L75

Recommendation 7

Change false at line 75 in PrePOMarketFactory.sol to allowed for better readability.

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

constructor() {} unused and should be removed. Doing so will improve readability and save some gas.