search

ubiquity / ubiquity-dollar

Ubiquity Dollar (uAD) smart contracts and user interface.
https://uad.ubq.fi
Apache License 2.0
31 stars 82 forks source link

Adding collateral enabled checks for redemption collections might make users uncapable of redeming their funds. #930

Open 0xadrii opened 2 months ago

0xadrii commented 2 months ago

[MEDIUM] - Adding collateral enabled checks for redemption collections might make users uncapable of redeming their funds.

This PR is intended to fix the bug described in this Sherlock issue. However, the Sherlock report is actually wrong and the vulnerability described must not be considered as an issue in the first place, so the collateralEnabled check should not be added on collecting redemptions.

Enabling/disabling collaterals allows the protocol to add or remove supported collaterals, not to be used under uncertain situations, such as a hack. In such type of situations, the isRedeemPaused check can be enabled to prevent users from redeeming if something wrong is currently happening.

The problem with adding the collateralEnabled check here is that if Ubiquity decides to disable a collateral in the future, some redemptions might be already queued, so users won't be able to withdraw their funds.

Recommendation: Remove the collateralEnabled check for collecting redemptions.

_Originally posted by @0xadrii in https://github.com/ubiquity/ubiquity-dollar/pull/894#discussion_r1572571068_

rndquu commented 2 months ago

The isRedeemPaused works pretty much the same way as collateralEnabled (one, two). So if some user requested collateral via redeemDollar() but hasn't collected it (because the protocol paused redeems) then the user also wouldn't be able to collect collateral regardless of whether the collateral is enabled or disabled.

Ideally we should remove both collateralEnabled and isRedeemPaused checks for collectRedemption() to allow users to withdraw any time. But since the protocol is in its early stage I would prefer things to be: 1) Highly centralized 2) With as many safety checks as possible

I understand your concerns but this issue is more about centralization risks. We'll gradually move to removing unnecessary safety checks.

I would move this issue to some metatask like "centralization risks" which would have issues like: 1) Transfer ownership (and admin role) to multisig or governance 2) Allow users to withdraw funds from the pool at any time

0xadrii commented 2 months ago

I agree with you, although from my point of view even if they work the same way they should be considered as two different mechanisms. From a security perspective, the pool will be protected by directly pausing redeems. But yes I get your point, and probably this won't be a problem for a long time until a collateral needs to be disabled , so I also agree that it could be handled as a centralization risk task and if needed then add it as a new feature.