sherlock-audit / 2024-05-kwenta-x-perennial-integration-update-judging

5 stars 3 forks source link

0xtenma - Tokens are not pushed to account after unwrapping in `MultiInvoker::_withdraw()` #15

Closed sherlock-admin3 closed 5 months ago

sherlock-admin3 commented 5 months ago

0xtenma

medium

Tokens are not pushed to account after unwrapping in MultiInvoker::_withdraw()

Summary

Tokens are not pushed to account after unwrapping in MultiInvoker::_withdraw()

Vulnerability Detail

When calling invoke() function, we call the _update() function and this function calls the _withdraw() function if the withdrawAmount is not 0, now if we look at _withdraw function it takes three parameters account, account and wrap to withdraw the funds. If wrap is true, it calls _unwrap function to unwrap DSU tokens. The issue is that it doesn't push the amount of USDC to account address like we have done in else block, pushing DSU amount to account address using DSU.push(account, UFixed18Lib.from(amount));.

MultiInvoker::_update():

    function _update(
        address account,
        IMarket market,
        UFixed6 newMaker,
        UFixed6 newLong,
        UFixed6 newShort,
        Fixed6 collateral,
        bool wrap,
        InterfaceFee memory interfaceFee1,
        InterfaceFee memory interfaceFee2
    ) internal isMarketInstance(market) {

        ... ...
        Fixed6 withdrawAmount = Fixed6Lib.from(Fixed18Lib.from(DSU.balanceOf()).sub(balanceBefore));

@>       if (!withdrawAmount.isZero()) _withdraw(account, withdrawAmount.abs(), wrap);
        ...
    }

From the above function we are calling MultiInvoker::_withdraw():

    function _withdraw(address account, UFixed6 amount, bool wrap) internal {
        if (wrap) {
            _unwrap(account, UFixed18Lib.from(amount));
            // @audit not pushed like deposit pull
        } else {
            DSU.push(account, UFixed18Lib.from(amount));
        }
    } 

Impact

Withdraw won't work properly after unwrapping USDC.

Code Snippet

https://github.com/sherlock-audit/2024-05-kwenta-x-perennial-integration-update/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L304C1-L310C6

Tool used

Manual Review

Recommendation

We recommend to add this line to send the USDC to the account.

    function _withdraw(address account, UFixed6 amount, bool wrap) internal {
        if (wrap) {
            _unwrap(account, UFixed18Lib.from(amount));
+           DSU.push(account, amount);
        } else {
            DSU.push(account, UFixed18Lib.from(amount));
        }
    } 
sherlock-admin3 commented 5 months ago

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

z3s commented:

Invalid; _unwrap sends USDCs: if (receiver != address(this)) USDC.push(receiver, UFixed6Lib.from(amount));

FSchmoede commented:

Incorrect. USDC is pushed to receiver as part of _unwrap.