code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L45

Vulnerability details

Description

In Collateral.sol, deposit and withdraw functions are subject to fees. They are either sent directly to the treasure in deposit / withdraw hooks, or are kept in the Collateral contract for safekeeping. Later, manager can use managerWithdraw function to collect the fees.

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);
}

Note the comment on deposit():

/**
 * @dev If hook not set, fees remain within the contract as extra reserves
 * (withdrawable by manager). Converts amount after fee from base token
 * units to collateral token units.
 */

Importantly, there is no bookkeeping of fees at all in deposit and withdraw functions. Ergo, manager can easily take non-fee reserves that are intended to be 1:1 mapped to minted tokens, leading to uncollateralized synthetic assets.

Impact

Admin may take non-fee baseTokens from Collateral.sol

Tools Used

Manual audit

Recommended Mitigation Steps

deposit() and withdraw() functions should be refactored, so that exact charged fee is stored, and only that fee may be taken by Manager. Otherwise, it is an excessive admin privilege scenario.

Picodes commented 1 year ago

That is not how I understand the comment, but I'll wait for the sponsor's input. To me if the hooks aren't set the fees remain within the contract as extra reserves, meaning they will be used to overcollateralize the contract, and won't be withdrawn later. The intent is that managerWithdraw is here to invest the funds, not for admin to withdraw fees.

c4-sponsor commented 1 year ago

ramenforbreakfast marked the issue as sponsor disputed

ramenforbreakfast commented 1 year ago

Yeah, so the comment is saying that if the fee is not taken by a hook, that it will simply reside in the contract and not be counted as a liability owed to depositors. Thus, it is not part of the funds subject to the manager's reserve req's.

I think the warden is confused because there is a misunderstanding that the manager should only be able to withdraw fees, which is not the case.

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

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