0xRobocop - Inconsistency handling of DAI as collateral in the BuyUSSDSellCollateral function

[sherlock-admin]

0xRobocop

high

Inconsistency handling of DAI as collateral in the BuyUSSDSellCollateral function

Summary

DAI is the base asset of the USSD.sol contract, when a rebalacing needs to occur during a peg-down recovery, collateral is sold for DAI, which then is used to buy USSD in the DAI / USSD uniswap pool. Hence, when DAI is the collateral, this is not sold because there no existe a path to sell DAI for DAI.

Vulnerability Detail

The above behavior is handled when collateral is about to be sold for DAI, see the comment no need to swap DAI (link to the code):

if (collateralval > amountToBuyLeftUSD) {
   // sell a portion of collateral and exit
   if (collateral[i].pathsell.length > 0) {
       uint256 amountBefore = IERC20Upgradeable(baseAsset).balanceOf(USSD);
       uint256 amountToSellUnits = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * ((amountToBuyLeftUSD * 1e18 / collateralval) / 1e18) / 1e18;
       IUSSD(USSD).UniV3SwapInput(collateral[i].pathsell, amountToSellUnits);
       amountToBuyLeftUSD -= (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
       DAItosell += (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
   } 
   else {
       // no need to swap DAI
       DAItosell = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * amountToBuyLeftUSD / collateralval;
   }
}

else {
   // @audit-issue Not handling the case where this is DAI as is done above.
   // sell all or skip (if collateral is too little, 5% treshold)
   if (collateralval >= amountToBuyLeftUSD / 20) {
      uint256 amountBefore = IERC20Upgradeable(baseAsset).balanceOf(USSD);
      // sell all collateral and move to next one
      IUSSD(USSD).UniV3SwapInput(collateral[i].pathsell, IERC20Upgradeable(collateral[i].token).balanceOf(USSD));
      amountToBuyLeftUSD -= (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
      DAItosell += (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
   }
}

The problem is in the else branch of the first if statement collateralval > amountToBuyLeftUSD, which lacks the check if (collateral[i].pathsell.length > 0)

Impact

A re-balancing on a peg-down recovery will fail if the collateralval of DAI is less than amountToBuyLeftUSD but greater than amountToBuyLeftUSD / 20 since the DAI collateral does not have a sell path.

Code Snippet

https://github.com/sherlock-audit/2023-05-USSD/blob/6d7a9fdfb1f1ed838632c25b6e1b01748d0bafda/ussd-contracts/contracts/USSDRebalancer.sol#L130-L139

Tool used

Manual Review

Recommendation

Handle the case as is the done on the if branch of collateralval > amountToBuyLeftUSD:

if (collateral[i].pathsell.length > 0) {
  // Sell collateral for DAI
}
else {
 // No need to swap DAI
}

[0xRobocop]

Escalate for 10 USDC

This is not a duplicate of #111

This issue points to an inconsistency in handling DAI as a collateral during peg-down recovery scenarios. The contract will try to sell DAI, but DAI does not have a sell path, so the transaction will revert.

if (collateralval > amountToBuyLeftUSD) {
   // sell a portion of collateral and exit
   if (collateral[i].pathsell.length > 0) {
       uint256 amountBefore = IERC20Upgradeable(baseAsset).balanceOf(USSD);
       uint256 amountToSellUnits = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * ((amountToBuyLeftUSD * 1e18 / collateralval) / 1e18) / 1e18;
       IUSSD(USSD).UniV3SwapInput(collateral[i].pathsell, amountToSellUnits);
       amountToBuyLeftUSD -= (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
       DAItosell += (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
   } 
   else {
       // no need to swap DAI
       DAItosell = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * amountToBuyLeftUSD / collateralval;
   }
}

else {
   // @audit-issue Not handling the case where this is DAI as is done above.
   // sell all or skip (if collateral is too little, 5% treshold)
   if (collateralval >= amountToBuyLeftUSD / 20) {
      uint256 amountBefore = IERC20Upgradeable(baseAsset).balanceOf(USSD);
      // sell all collateral and move to next one
      IUSSD(USSD).UniV3SwapInput(collateral[i].pathsell, IERC20Upgradeable(collateral[i].token).balanceOf(USSD));
      amountToBuyLeftUSD -= (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
      DAItosell += (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
   }
}

See the inconsistency on the upper if and else branches. The else branch may try to sell DAI, but DAI does not have a sell path.

[sherlock-admin]

Escalate for 10 USDC

This is not a duplicate of #111

This issue points to an inconsistency in handling DAI as a collateral during peg-down recovery scenarios. The contract will try to sell DAI, but DAI does not have a sell path, so the transaction will revert.

if (collateralval > amountToBuyLeftUSD) {
   // sell a portion of collateral and exit
   if (collateral[i].pathsell.length > 0) {
       uint256 amountBefore = IERC20Upgradeable(baseAsset).balanceOf(USSD);
       uint256 amountToSellUnits = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * ((amountToBuyLeftUSD * 1e18 / collateralval) / 1e18) / 1e18;
       IUSSD(USSD).UniV3SwapInput(collateral[i].pathsell, amountToSellUnits);
       amountToBuyLeftUSD -= (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
       DAItosell += (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
   } 
   else {
       // no need to swap DAI
       DAItosell = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * amountToBuyLeftUSD / collateralval;
   }
}

else {
   // @audit-issue Not handling the case where this is DAI as is done above.
   // sell all or skip (if collateral is too little, 5% treshold)
   if (collateralval >= amountToBuyLeftUSD / 20) {
      uint256 amountBefore = IERC20Upgradeable(baseAsset).balanceOf(USSD);
      // sell all collateral and move to next one
      IUSSD(USSD).UniV3SwapInput(collateral[i].pathsell, IERC20Upgradeable(collateral[i].token).balanceOf(USSD));
      amountToBuyLeftUSD -= (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
      DAItosell += (IERC20Upgradeable(baseAsset).balanceOf(USSD) - amountBefore);
   }
}

See the inconsistency on the upper if and else branches. The else branch may try to sell DAI, but DAI does not have a sell path.

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.

[hrishibhat]

@ctf-sec

[hrishibhat]

Result:
Medium
Has duplicates

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• 0xRobocop: accepted