bin2chen - sellCollateral() using incorrect parameters when calling getAsset

[sherlock-admin4]

bin2chen

medium

sellCollateral() using incorrect parameters when calling getAsset

Summary

sellCollateral() using incorrect parameters when calling getAsset

Vulnerability Detail

BBLeverage.sellCollateral() the code is follows:

    function sellCollateral(address from, uint256 share, bytes calldata data)
        external
        optionNotPaused(PauseType.LeverageSell)
        solvent(from, false)
        notSelf(from)
        returns (uint256 amountOut)
    {
...
        amountOut = leverageExecutor.getAsset(
@>          assetId, address(collateral), address(asset), memoryData.leverageAmount, from, data
        );
        memoryData.shareOut = yieldBox.toShare(assetId, amountOut, false);
        address(asset).safeApprove(address(yieldBox), type(uint256).max);

In the function call getAsset(assetId, address(collateral)...), the second parameter is passed as collateral, but it should actually be asset.

interface ILeverageExecutor {
..
    function getAsset(
        uint256 assetId,
@>      address assetAddress,
        address collateralAddress,
        uint256 collateralAmountIn,
        address from,
        bytes calldata data
    ) external returns (uint256 assetAmountOut); //used for sellCollateral

Impact

Incorrect usage of getAsset() can lead to exchange failures.

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/bigBang/BBLeverage.sol#L144

Tool used

Manual Review

Recommendation

    function sellCollateral(address from, uint256 share, bytes calldata data)
        external
        optionNotPaused(PauseType.LeverageSell)
        solvent(from, false)
        notSelf(from)
        returns (uint256 amountOut)
    {
        if (address(leverageExecutor) == address(0)) {
            revert LeverageExecutorNotValid();
        }
        _allowedBorrow(from, share);
        _removeCollateral(from, address(this), share);

        _SellCollateralMemoryData memory memoryData;

        (, memoryData.obtainedShare) =
            yieldBox.withdraw(collateralId, address(this), address(leverageExecutor), 0, share);
        memoryData.leverageAmount = yieldBox.toAmount(collateralId, memoryData.obtainedShare, false);
        amountOut = leverageExecutor.getAsset(
-           assetId, address(collateral), address(asset), memoryData.leverageAmount, from, data
+           assetId, address(asset), address(collateral), memoryData.leverageAmount, from, data
        );

Duplicate of #115

[sherlock-admin4]

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

[CergyK]

Escalate

This should be a duplicate of #115, the interface used for getAsset is wrong

[sherlock-admin2]

Escalate

This should be a duplicate of #115, the interface used for getAsset is wrong

You've created a valid escalation!

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.

[nevillehuang]

I believe they are separate issues, this seems to be an issue relating to the correct use of the interface with 6 arguments but inputting the wrong parameter whereas the other issue is talking about wrong use of interface.

[CergyK]

I believe they are separate issues, this seems to be an issue relating to the correct use of the interface with 6 arguments but inputting the wrong parameter whereas the other issue is talking about wrong use of interface.

Yes sorry my message was a bit short, I meant since the whole set of arguments needs to be changed anyway, the fix to #115 will also most likely fix this one (which IIRC is a criteria for duplication).

[cvetanovv]

I agree with @CergyK escalation, and we can duplicate them. The root of these issues is the leverageExecutor interface, which is incorrect.

[cvetanovv]

Planning to accept the escalation and duplicate with #115.

[Evert0x]

Result: Medium Duplicate of #115

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• CergyK: accepted