sellCollateral() when sell collateral, the quantity parameter passed may too large

Summary

In the sellCollateral() due to rounding down, after executing yieldBox.withdraw(shares), subsequently calling yieldBox.toAmount(shares) returns the quantity that is 1 unit larger than the previous yieldBox.withdraw(). This difference leads to insufficient balance in subsequent exchanges.

Vulnerability Detail

The current implementation of sellCollateral() is as follows:

    function sellCollateral(address from, uint256 share, bytes calldata data)
        external
        optionNotPaused(PauseType.LeverageSell)
        solvent(from, false)
        notSelf(from)
        returns (uint256 amountOut)
    {
...
        _allowedBorrow(calldata_.from, calldata_.share);
        _removeCollateral(calldata_.from, address(this), calldata_.share);

@>      yieldBox.withdraw(collateralId, address(this), address(leverageExecutor), 0, calldata_.share);
@>      uint256 leverageAmount = yieldBox.toAmount(collateralId, calldata_.share, false);
@>      amountOut = leverageExecutor.getAsset(
            assetId, address(collateral), address(asset), leverageAmount, calldata_.from, calldata_.data
        );
...

leverageAmount recalculates using yieldBox.toAmount(). This value may be greater by 1 than the actual amount retrieved through yieldBox.withdraw().

Example:

totalShare = 4171824492 totalAsset = 4171824494 withdrawShares = 2931277435

yieldBox.withdraw(withdrawShares) -> round down
- get_assets = withdrawShares totalAsset / totalShare = 2931277435 4171824494 / 4171824492 = 2931277436
then yieldBox.toAmount(withdrawShares) ->round down
- return_amount= withdrawShares ( totalAsset - get_assets) / (totalShare - withdrawShares) = 2931277435 (4171824494 - 2931277436) / (4171824492 - 2931277435) = 2931277437

return_amount - get_assets = 2931277437 - 2931277436 = 1

Calling leverageExecutor.getAsset(leverageAmount) will result in insufficient collateral, because leverageAmount > 1.

You can use the following fuzz to obtain other test values.

  function testFuzz_shares(uint256 asset,uint256 totalShares,uint256 withdrawShares) external {
    vm.assume(totalShares> 1000000); 
    vm.assume(totalShares< 100e18); 
    vm.assume(asset > totalShares); 
    vm.assume(asset - totalShares < 10000); 
    vm.assume(withdrawShares < totalShares);

    uint256 getAsset = withdrawShares * asset / totalShares;
    uint256 asset2 = withdrawShares *  (asset - getAsset) / (totalShares - withdrawShares); 
    console2.log(asset);
    require(getAsset == asset2,"inv");

$ forge test -vvv

Traces:
  [8052] CounterTest::testFuzz_shares(4171824494 [4.171e9], 4171824492 [4.171e9], 2931277435 [2.931e9])

Impact

The withdraw collateral quantity small then the collateral quantity parameter used for exchange, which leads to revert.

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/singularity/SGLLeverage.sol#L128

Tool used

Manual Review

Recommendation

Don't recalculate, just take the return value of yieldBox.withdraw.

    function sellCollateral(address from, uint256 share, bytes calldata data)
        external
        optionNotPaused(PauseType.LeverageSell)
        solvent(from, false)
        notSelf(from)
        returns (uint256 amountOut)
    {
...
-       yieldBox.withdraw(collateralId, address(this), address(leverageExecutor), 0, calldata_.share);
-       uint256 leverageAmount = yieldBox.toAmount(collateralId, calldata_.share, false);
+       (uint256 leverageAmount ,) = yieldBox.withdraw(collateralId, address(this), address(leverageExecutor), 0, calldata_.share);
        amountOut = leverageExecutor.getAsset(
            assetId, address(collateral), address(asset), leverageAmount, calldata_.from, calldata_.data
        );
        uint256 shareOut = yieldBox.toShare(assetId, amountOut, false);

Duplicate of #61

sherlock-audit / 2024-02-tapioca-judging

bin2chen - sellCollateral() when sell collateral, the quantity parameter passed may too large #105