prePO contest - Madalad's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L81 https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/Collateral.sol#L112-L115

Vulnerability details

Impact

managerWithdraw() relies on a calling ManagerWithdrawHook.hook() to ensure that the baseToken reserve does not fall below the minimum.

If the withdrawHook address is the zero address, the call to hook() is skipped. Since there is no check within setManagerWithdrawHook(), the owner can set withdrawHook to the zero address, and then call managerWithdraw() to withdraw Collateral's entire balance (potentially in the same transaction).

Alternatively, the same outcome can be achieved by calling ManagerWithdrawHook.setMinReservePercentage() and passing 0 as the parameter, immediately before calling managerWithdraw().

According to ICollateral.sol, this should not be possible.

Proof of Concept

it("allows deployer to drain the contract", async () => {
    expect(
    await collateral.connect(deployer).managerWithdraw(await collateral.getReserve())
    ).to.be.revertedWith('reserve would fall below minimum')

    // set hook to zero address
    await collateral.connect(deployer).setWithdrawHook(ZERO_ADDRESS)
    expect(await collateral.getWithdrawHook()).to.eq(ZERO_ADDRESS)

    // withdraw entire baseToken balance
    await collateral.connect(deployer).managerWithdraw(await collateral.getReserve())
    expect(await collateral.getReserve()).to.eq(0)
})

Tools Used

Hardhat

Recommended Mitigation Steps

Can do one or more of the following:

• Add require(_newManagerWithdrawHook != address(0)) to setManagerWithdrawHook()
• Add require(_newMinReservePercentage != 0) to setMinReservePercentage()
• Use a timelocked DAO/multisig for admin functions