feat: check if collateral is enabled in collectRedemption #894
Conversation
Also add a unit test that verifies the check. Resolves: sherlock-audit/2023-12-ubiquity-judging#29
|
This is wrong to push all fixes (low and medium/high) to the same |
* fix: deprecate incentive on dollar token * chore: update * chore: update * Fix set role admin (#880) * feat: add setRoleAdmin to AccessControlFacet The setRoleAdmin can be only accessed by the admin. * feat: add setRoleAdmin to AccessControl interface * test: add testSetRoleAdmin_ShouldSetAdminRoleForGivenRole test * feat: update access control for setRoleAdmin * test: fix ShouldSetAdminRoleForGivenRole and add test for revert * feat: add getRedeemCollateralBalance() method * fix: remove balanced reserves check (#883) * fix: limit AMO minter borrow amount (#882) * fix: limit AMO minter borrow amount * test: assert free collateral amount * fix: do not allow to mint dollar with zero collateral Resolves: sherlock-audit/2023-12-ubiquity-judging#207 * test: add testMintDollar_ShouldRevert_IfZeroCollateralAvailable * test: update comment in zero collateral mint test * Update block count in a week (#891) * feat: implement BlocksInWeek script task The BlocksInWeek task provides a very close approximate of number of blocks mined during one week. Supported networks: mainnnet, sepolia. Example usage: npx tsx scripts/task/task.ts BlocksInWeek --network=mainnet npx tsx scripts/task/task.ts BlocksInWeek --network=sepolia Resolves: sherlock-audit/2023-12-ubiquity-judging#230 * feat: update weekly block count to 49930 Set weekly block count to 49930 as measured in February 2024 npx tsx scripts/task/task.ts BlocksInWeek --network=mainnet ... Calculating number of blocks in the last week... Recent average block time: 12 seconds Estimated blocks in a week best case 50400 Produced 49930 blocks, 470 worst than the best case Resolves: sherlock-audit/2023-12-ubiquity-judging#230 * feat: rename task function to funcBlocksInAWeek As proposed during pull request review. * feat: use CurveStableSwapMetaNG contract * refactor: update migrations to use latest metapool * refactor: deprecate IMetaPool * refactor: remove MockTWAPOracleDollar3pool * refactor: remove TWAPOracleDollar3poolFacet * refactor: remove MockMetaPool * refactor: remove LibTWAPOracle * fix(dapp): remove TWAP oracle ABI import * refactor(frontend): use ICurveStableSwapMetaNG for TWAP * feat: check if collateral is enabled in collectRedemption (#894) Also add a unit test that verifies the check. Resolves: sherlock-audit/2023-12-ubiquity-judging#29 --------- Co-authored-by: molecula451 <[email protected]> Co-authored-by: korrrba <[email protected]> Co-authored-by: Korrrba <[email protected]>
| @@ -491,7 +491,11 @@ library LibUbiquityPool { | |||
| */ | |||
| function collectRedemption( | |||
| uint256 collateralIndex | |||
| ) internal returns (uint256 collateralAmount) { | |||
| ) | |||
There was a problem hiding this comment.
[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.
Also add a unit test that verifies the check.
Resolves: sherlock-audit/2023-12-ubiquity-judging#29