search

code-423n4 / 2022-12-prepo-findings

0 stars 1 forks source link

Misconfigured or malicious MANAGER Can drain, lose or steal ALL of the collateral. #335

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L80-L83 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L85-L88 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L17

Vulnerability details

Impact

Collateral.sol allows withdrawal of funds to an arbitrary manager account. There are no inherent limitation to:

  1. identity of manager address
  2. withdrawable amount

Details of 1. : manager setter ` is access controlled still may be misconfigured or a malicious admin may exploit it. It may happen as follwoing: a)managerset to a non upgradeable contract with no withdraw functionality. FUNDS PERMANENTLY LOST b)managerset to an account unassociated with the protocol. FUNDS STOLEN c)manager` set to a legitimate strategy, which loses funds due to bad risk management

Details of 2: max withdrawable amount is being arbitrarily set by access controlled function ``. Which means it may very well be set to 100%. ALL FUNDS COULD BE LOST

Proof of Concept

It's even an exploit. It's a design flow. Tests already included in the repo demonstrate the crux of the issue eg

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/test/Collateral.test.ts#L641-L658

Tools Used

Pen and paper

Recommended Mitigation Steps

Develop the concept of manager into something approximating yield aggregator strategy:

  1. Design an interface for a strategy
  2. Enforce manager to implement it
  3. Let users depositing the collateral to chose the strategy they would like to use OR Let governance to vet and chose strategies for particular collaterals.
Picodes commented 1 year ago

This is not a vulnerability but a design choice. Not that your mitigation 1 and 2 do not increase the security, a contract could respect an interface but still do anything

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid