Ether can be locked in the PoolFactory contract without a way to retrieve it

[code423n4]

Handle

broccolirob

Vulnerability details

If a borrower calls the createPool function with a non-zero value, but also includes an ERC20 token address for _collateralToken, then the Ether value sent will be locked in the PoolFactory contract forever.

• createPool L260-317

In the _createPool function, a _collateralToken address other than the zero address will set the amount variable to zero. That amount variable will be passed to create2 which will send 0 wei to the newly created Pool contract.

// _createPool L349
uint256 amount = _collateralToken == address(0) ? _collateralAmount : 0;

Impact

A borrower can accidentally lock Ether in the PoolFactory without the ability to retrieve it.

Proof of Concept

A borrower reuses a script they made to create a pool and deposit collateral. They intend to deposit Ether as collateral so they send value with the transaction, but forget to change the _collateralToken address to address(0). The Pool contract will be deployed using the _collateralToken, and will lock the Ether sent in the PoolFactory

Tools Used

Manual analysis and Hardhat.

Recommended Mitigation Steps

If msg.value is greater than 0, make sure the _collateralToken address is set to address(0).

[ritik99]

We will add this check but the scenario laid out is more about sanity checks on the side of the end-user. Assets are not stolen or compromised directly but because of user error. Such cases are better handled via UI/UX. We would suggest a (1) Low rating given the likelihood

[0xean]

Marking down to medium risk based on the c4 documentation and some external requirements on how this would have to occur.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.