code-423n4 / 2022-03-lifinance-findings

6 stars 4 forks source link

GenericSwapFacet misuses _lifiData #138

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22

Vulnerability details

Impact

https://github.com/code-423n4/2022-03-lifinance/blob/main/docs/GenericSwapFacet.md stated that _lifiData is strictly for analytics purposes. But _lifiData is used to set receivingAsset.

Proof of Concept

In GenericSwapFacet.swapTokensGeneric, _lifiData.receivingAssetId is used in LibAsset.getOwnBalance and LibAsset.transferAsset.

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22

    function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData) public payable {
        uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(_lifiData.receivingAssetId) - receivingAssetIdBalance;

        LibAsset.transferAsset(_lifiData.receivingAssetId, payable(msg.sender), postSwapBalance);
        ...
    }

Tools Used

Manual code review.

Recommended Mitigation Steps

In order to follow the policy, there should be a new parameter GenericSwapData.

    function swapTokensGeneric(LiFiData memory _lifiData, LibSwap.SwapData[] calldata _swapData, GenericSwapData _genericSwapData) public payable {
        uint256 receivingAssetIdBalance = LibAsset.getOwnBalance(_genericSwapData.receivingAssetId);

        // Swap
        _executeSwaps(_lifiData, _swapData);

        uint256 postSwapBalance = LibAsset.getOwnBalance(_genericSwapData.receivingAssetId) - receivingAssetIdBalance;

        LibAsset.transferAsset(_genericSwapData.receivingAssetId, payable(msg.sender), postSwapBalance);
        ...
    }
H3xept commented 2 years ago

Duplicate of #62

gzeoneth commented 2 years ago

Agree with sponsor this does not pose a functional risk. Changing to Low/QA.

gzeoneth commented 2 years ago

Consider with warden's QA Report #139

gzeoneth commented 2 years ago

This should be a duplicate of https://github.com/code-423n4/2022-03-lifinance-findings/issues/75 instead, I made a mistake during the deduplication process.