code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Description

In PrePOMarket, during withdrawal, if there is a redeemHook set it is called in order to charge withdraw fees:

if (address(_redeemHook) != address(0)) {
  collateral.approve(address(_redeemHook), _expectedFee);
  uint256 _collateralAllowanceBefore = collateral.allowance(address(this), address(_redeemHook));
  _redeemHook.hook(msg.sender, _collateralAmount, _collateralAmount - _expectedFee);
  _actualFee = _collateralAllowanceBefore - collateral.allowance(address(this), address(_redeemHook));
  collateral.approve(address(_redeemHook), 0);
} else { _actualFee = 0; }

Unfortunately, this flow introduces a serious centralization issue. Owner can easily freeze all funds by setting _redeemHook to invalid address (like 0x01). The _redeemHook.hook() call will revert and trip the withdrawl. No users may complete withdraw process. Malicious admin can even further abuse it and selectively allow certain users, maybe inside traders, to withdraw by setting the redeemHook properly only momentarily.

function setRedeemHook(IMarketHook redeemHook) external override onlyOwner {
  _redeemHook = redeemHook;
  emit RedeemHookChange(address(redeemHook));
}

Impact

Admin has excessive privilege and can freeze PrePOMarket withdrawals indefinitely.

Tools Used

Manual audit

Recommended Mitigation Steps

Withdraw is a delicate function which should always be available to users. It would be best to refactor the code so that when redeemHook fails, the withdraw would still carry through, using try/catch statement.

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-sponsor commented 1 year ago

ramenforbreakfast marked the issue as sponsor disputed

ramenforbreakfast commented 1 year ago

We are not considering any issues that rely breaking trust assumptions. If so, then any usage of a hook within our architecture could be raised following the same logic, which is why I am considering this issue to be invalid.

Also markets are completely liquid on Uniswap AMMs for the duration of the market, only at the very end is redeem a concern for those who wish to settle according to the final payout distribution set by governance.

Picodes commented 1 year ago

Downgrading to QA as this risk is known from scratch, considering the owner can achieve the same behavior using this line. Therefore there is no real privilege escalation.