search

sherlock-audit / 2024-02-tapioca-judging

2 stars 2 forks source link

duc - `getCollateral` and `getAsset` functions of the AssetTotsDaiLeverageExecutor contract decode data incorrectly #84

Open sherlock-admin3 opened 5 months ago

sherlock-admin3 commented 5 months ago

duc

medium

getCollateral and getAsset functions of the AssetTotsDaiLeverageExecutor contract decode data incorrectly

Summary

See vulnerability detail

Vulnerability Detail

In AssetTotsDaiLeverageExecutor contract, getCollateral function decodes the data before passing it to _swapAndTransferToSender function.

SLeverageSwapData memory swapData = abi.decode(data, (SLeverageSwapData));
uint256 daiAmount =
    _swapAndTransferToSender(false, assetAddress, daiAddress, assetAmountIn, swapData.swapperData);

However, _swapAndTransferToSender will decode this data again to obtain the swapperData:

function _swapAndTransferToSender(
    bool sendBack,
    address tokenIn,
    address tokenOut,
    uint256 amountIn,
    bytes memory data
) internal returns (uint256 amountOut) {
    SLeverageSwapData memory swapData = abi.decode(data, (SLeverageSwapData));
    ...

The redundant decoding will cause the data to not align as expected, which is different from SimpleLeverageExecutor.getCollateral() function (code snippet)

Impact

getCollateral and getAsset of AssetTotsDaiLeverageExecutor will not work as intended due to incorrectly decoding data.

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/leverage/AssetTotsDaiLeverageExecutor.sol#L53-L55 https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/markets/leverage/AssetTotsDaiLeverageExecutor.sol#L88-L91

Tool used

Manual Review

Recommendation

The AssetTotsDaiLeverageExecutor contract should pass data directly to _swapAndTransferToSender, similar to the SimpleLeverageExecutor contract

sherlock-admin2 commented 5 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

it seem to have been encoded twice, cuz it will not work if its encoded once in the first place

nevillehuang commented 5 months ago

@cryptotechmaker This seems to lack an impact description, but would this cause an revert within _swapAndTransferToSender?

cryptotechmaker commented 5 months ago

Yes, it causes a revert.

sherlock-admin4 commented 5 months ago

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