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 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol#L17

Vulnerability details

Impact

In the managerWithdraw function on Line80 of Collateral.sol, user with MANAGER_WITHDRAW_ROLE can withdraw funds from Collateral vault subject to a minimum reserve requirement condition on Line 17 of ManagerWithdrawHook.sol

At this stage, we are not checking if the balance left after withdrawal is sufficient to honor any collateral withdrawals by current LPs.

In the extreme case where minimumReserve is set to a low number OR global/user withdrawal cap per period is set to a high number OR there is a case of sudden spike in withdrawals, right after a manager withdrawal (or combination of all the above), we could end up in a scenario where minimum Reserve after withdrawal is less than 'withdrawal request' of users in a single period.

In such cases, users will face a denial of service because of insufficient balance. Ofcourse the edge case here is dependent on userWithdrawalCapPerPeriod and minimumReserve parameters set by the protocol. But such a scenario is conceptually possible.

Since the event can potentially stop the normal functioning of protocol & considering it to be an edge case, I've marked it as MEDIUM risk.

Proof of Concept

Suppose vault had 10 depositors of 100 USDC each. userWithdrawLimitPerPeriod is 80 USDC in a single period and minimumReserve is 5%. A user with MANAGER_WITHDRAW_ROLE withdraws 95% of base tokens from vault leaving just 50 USDC in collateral vault. Bob, a depositor, tries to withdraw maximum of 80 USDC, and gets rejected because of insufficient balance in the vault.

Tools Used

Recommended Mitigation Steps

Just like globalNetDepositAmount being tracked in DepositRecord contract in line 11 of DepositRecord.sol, a globalUserDepositAmount should be tracked that is a cumulative sum of all user deposits (excluding deposits made by protocol itself).

In the managerFunction on Line 82 in Collateral.sol, following code block can be changed from

  function managerWithdraw(uint256 _amount) external override onlyRole(MANAGER_WITHDRAW_ROLE) nonReentrant {
    if (address(managerWithdrawHook) != address(0)) managerWithdrawHook.hook(msg.sender, _amount, _amount);
    baseToken.transfer(manager, _amount);
  }

to

  function managerWithdraw(uint256 _amount) external override onlyRole(MANAGER_WITHDRAW_ROLE) nonReentrant {
    if (address(managerWithdrawHook) != address(0)) managerWithdrawHook.hook(msg.sender, _amount, _amount);

   if(minimumReserve * depositRecord. getGlobalNetDepositAmount() >= depositRecord.getGlobalUserDepositAmount()){
       baseToken.transfer(manager, _amount); // assumed that `getGlobalUserDepositAmount()` getter would exist in DepositRecord
   }
  }.

By checking if minimum reserves are sufficient to meet all user withdrawal requests, we can mitigate the risk stated above.

Picodes commented 1 year ago

This is the intended behavior as the manager is supposed to take funds to invest them

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

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