Skip to content
This repository has been archived by the owner on Jul 14, 2024. It is now read-only.

Krace - Users can still redeem their assets even after the collateral has been disabled. #29

Closed
sherlock-admin opened this issue Jan 10, 2024 · 0 comments
Labels
Non-Reward This issue will not receive a payout

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Jan 10, 2024

Krace

high

Users can still redeem their assets even after the collateral has been disabled.

Summary

The collectRedemption function, responsible for collecting collateral tokens after redeeming Ubiquity Dollars, currently lacks a check to verify whether the collateral has been disabled. This omission could allow users to redeem disabled collateral from the contract.

Vulnerability Detail

Redeem process is split in two steps:

  1. redeemDollar() will burn users' Ubiquity Dollar and record the amounts of collateral users could receive.
    function redeemDollar(
        uint256 collateralIndex,
        uint256 dollarAmount,
        uint256 collateralOutMin
    )
        internal
        //@audit users can not redeem disabled collateral
        collateralEnabled(collateralIndex)
        returns (uint256 collateralOut)
    {
  1. collectRedemption() send the collateral to the users.
    function collectRedemption(
        uint256 collateralIndex
//@audit If the collateral is disabled after redeemDollar, users could still receive the collateral
    ) internal returns (uint256 collateralAmount) {

However, only the first function, redeemDollar, checks if the collateral is enabled. This means that if a collateral is disabled after redeemDollar, users could still obtain the collateral through the collectRedemption function.

Impact

Users might have the ability to redeem disabled collateral from the contract.

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L399-L406

https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L476-L478

Tool used

Manual Review

Recommendation

It's recommended to add a check in the function collectRedemption.

diff --git a/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol b/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol
index 2f2a90a..8bf074c 100644
--- a/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol
+++ b/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol
@@ -475,7 +475,7 @@ library LibUbiquityPool {
      */
     function collectRedemption(
         uint256 collateralIndex
-    ) internal returns (uint256 collateralAmount) {
+    ) internal collateralEnabled(collateralIndex) returns (uint256 collateralAmount) {
         UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

         require(

Duplicate of #23

@github-actions github-actions bot added Excluded Excluded by the judge without consulting the protocol or the senior Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label and removed Excluded Excluded by the judge without consulting the protocol or the senior labels Jan 14, 2024
@sherlock-admin sherlock-admin changed the title Genuine Carbon Snake - Users can still redeem their assets even after the collateral has been disabled. Krace - Users can still redeem their assets even after the collateral has been disabled. Jan 24, 2024
@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Jan 24, 2024
gitcoindev added a commit to gitcoindev/ubiquity-dollar that referenced this issue Feb 6, 2024
molecula451 pushed a commit to ubiquity/ubiquity-dollar that referenced this issue Feb 6, 2024
molecula451 added a commit to ubiquity/ubiquity-dollar that referenced this issue Mar 1, 2024
* 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 <web3samuraidev@gmail.com>
Co-authored-by: korrrba <88761781+gitcoindev@users.noreply.github.com>
Co-authored-by: Korrrba <gitcoiner@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Non-Reward This issue will not receive a payout
Projects
None yet
Development

No branches or pull requests

1 participant