Skip to content
This repository has been archived by the owner on Nov 26, 2023. It is now read-only.

T1MOH - BuyUSSDSellCollateral() always sells 0 amount if need to sell part of collateral #656

Open
sherlock-admin opened this issue May 24, 2023 · 5 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented May 24, 2023

T1MOH

high

BuyUSSDSellCollateral() always sells 0 amount if need to sell part of collateral

Summary

Due to rounding error there is misbehaviour in BuyUSSDSellCollateral() function. It results in selling 0 amount of collateral.

Vulnerability Detail

Suppose the only collateral in protocol is 1 WBTC; 1 WBTC costs 30_000 USD;
UniV3Pool DAI/ USSD has following liquidity: (3000e6 USSD, 2000e18 DAI)
And also USSD is underpriced so call rebalance:

    function rebalance() override public {
      uint256 ownval = getOwnValuation(); // it low enough to dive into if statement (see line below) 
      (uint256 USSDamount, uint256 DAIamount) = getSupplyProportion(); // (3000e6 USSD, 2000e18 DAI)
      if (ownval < 1e6 - threshold) {
        // peg-down recovery
        BuyUSSDSellCollateral((USSDamount - DAIamount / 1e12)/2); //  500 * 1e6     = (3000e6 - 2000e18 / 1e12) / 2

Take a look into BuyUSSDSellCollateral (follow comments):

    function BuyUSSDSellCollateral(uint256 amountToBuy) internal { // 500e6
      CollateralInfo[] memory collateral = IUSSD(USSD).collateralList();
      //uint amountToBuyLeftUSD = amountToBuy * 1e12 * 1e6 / getOwnValuation();
      uint amountToBuyLeftUSD = amountToBuy * 1e12; // 500e18
      uint DAItosell = 0;
      // Sell collateral in order of collateral array
      for (uint256 i = 0; i < collateral.length; i++) {
        // 30_000e18 = 1e8 * 1e18 / 10**8 * 30_000e18 / 1e18
        uint256 collateralval = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * 1e18 / (10**IERC20MetadataUpgradeable(collateral[i].token).decimals()) * collateral[i].oracle.getPriceUSD() / 1e18;
        if (collateralval > amountToBuyLeftUSD) {
          // sell a portion of collateral and exit
          if (collateral[i].pathsell.length > 0) {
            uint256 amountBefore = IERC20Upgradeable(baseAsset).balanceOf(USSD); // 0
            // amountToSellUnits = 1e8 * ((500e18 * 1e18 / 30_000e18) / 1e18) / 1e18 = 1e8 * (0) / 1e18 = 0
            uint256 amountToSellUnits = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * ((amountToBuyLeftUSD * 1e18 / collateralval) / 1e18) / 1e18;
            // and finally executes trade of 0 WBTC
            IUSSD(USSD).UniV3SwapInput(collateral[i].pathsell, amountToSellUnits);
            amountToBuyLeftUSD -= (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore); // 0 = 0 - 0
            DAItosell += (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore); // 0 += 0
            ...

So protocol will not buy DAI and will not sell DAI for USSD in UniswapV3Pool to support peg of USSD to DAI

Impact

Protocol is not able of partial selling of collateral for token. It block algorithmic pegging of USSD to DAI

Code Snippet

https://github.com/sherlock-audit/2023-05-USSD/blob/main/ussd-contracts/contracts/USSDRebalancer.sol#L121

Tool used

Manual Review, VS Code

Recommendation

Refactor formula of amountToSellUnits

// uint256 amountToSellUnits = (decimals of collateral) * (DAI amount to get for sell) / (price of 1 token of collateral)
uint256 amountToSellUnits = collateral[i].token).decimals() * amountToBuyLeftUSD / collateral[i].oracle.getPriceUSD()
@github-actions github-actions bot closed this as completed Jun 5, 2023
@github-actions github-actions bot added High A valid High severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Jun 5, 2023
@sherlock-admin sherlock-admin added Medium A valid Medium severity issue Reward A payout will be made for this issue and removed High A valid High severity issue labels Jun 23, 2023
@T1MOH593
Copy link

Escalate for 10 USDC

This is not a duplicate of #111
This report describes that partially collateral can't be sold, because amountToSellUnits is 0 due to rounding issue. Noticed #183 is similar to my issue

@sherlock-admin
Copy link
Contributor Author

Escalate for 10 USDC

This is not a duplicate of #111
This report describes that partially collateral can't be sold, because amountToSellUnits is 0 due to rounding issue. Noticed #183 is similar to my issue

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Jun 23, 2023
@ctf-sec
Copy link
Collaborator

ctf-sec commented Jun 29, 2023

I agree this issue and #183 are not duplicate of #111 and can be grouped together as a new valid medium, will see if this is a duplicate of other issue

@hrishibhat hrishibhat reopened this Jul 14, 2023
@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout Reward A payout will be made for this issue and removed Reward A payout will be made for this issue Non-Reward This issue will not receive a payout labels Jul 14, 2023
@hrishibhat hrishibhat 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 Reward A payout will be made for this issue labels Jul 14, 2023
@sherlock-admin sherlock-admin added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Jul 14, 2023
@hrishibhat
Copy link
Contributor

hrishibhat commented Jul 14, 2023

Result:
Medium
Has duplicates

@sherlock-admin
Copy link
Contributor Author

sherlock-admin commented Jul 14, 2023

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected and removed Escalated This issue contains a pending escalation labels Jul 14, 2023
@sherlock-admin2 sherlock-admin2 added the Has Duplicates A valid issue with 1+ other issues describing the same vulnerability label Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

5 participants