search

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

5 stars 3 forks source link

0xumarkhatab - `PerennialAction.UPDATE_POSITION` invoke action should have deadline checks in place #5

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

0xumarkhatab

high

PerennialAction.UPDATE_POSITION invoke action should have deadline checks in place

Summary

The invoke method should have slippage checks involved because PerennialAction.UPDATE_POSITION action involves converting usdc to dsu that might be desirable for user only within some time frame

Vulnerability Detail

Their can be scenarios when the user would want to wrap his USDC to DSU only if the transaction is profitable because he might have the window of opportunity for some time when he can invest those DSU at some place and get more in return .

But imagine the USDC value is 1$ and user wants to wrap his 1 Billion USDC for 1 Billion DSU in the next block and he wanted to invest those 1 Billion DSU at some other place where that opportunity is only open for that block.

Imagine user initiates the transaction of His wrap of 1 Billion USDC worth each 1$ , the transaction gets delayed due to network congestion or other network scenarios and the user gets 1 Billion DSU after 4 blocks .

This led to losing is opportunity that he wanted the wrap for at the first place.

Situation escalates when he now unwraps them but the price of USDc deops to 0.95$

which means his 1 Billion USDC will be now worth 0.95 Billion $

effectively making him suffer a Loss of 50000000$ or 50 Million$ which is huge

A great thing would be to allow a deadline parameter to only allow the transaction to happen if he deadline has not been passed

This will , in our example would save him 50M$ and not allow the swap if the opportunity has gone .

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

        if (collateral.sign() == 1) _deposit(account, collateral.abs(), wrap);

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

Impact

Loss of user funds value

Code Snippet

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

Tool used

Manual Review

Recommendation

Add a deadline parameter to avoid transaction execution beyond acceptable time frames by users

sherlock-admin2 commented 5 months ago

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

z3s commented:

Low/Info; From README: DSU - TRUSTED

takarez commented:

this seem to be 1:1 with the stabes.

0xumarkhatab commented 5 months ago

@z3s thank you for co-operating.

I believe my earliest example still suffice to show the loss of funds as you described in the discord thread .

From Criteria for Issue Validity: Slippage related issues showing a definite loss of funds with a detailed explanation for the same can be considered valid high. So here is my detailed explanation

imagine the USDC value is 1$ and user wants to wrap his 1 Billion USDC for 1 Billion DSU in the next block and he wanted to invest those 1 Billion DSU at some other place where that opportunity is only open for that block.

Imagine user initiates the transaction of His wrap of 1 Billion USDC worth each 1$ , the transaction gets delayed due to network congestion or other network scenarios and the user gets 1 Billion DSU after 4 blocks .

This led to losing is opportunity that he wanted the wrap for at the first place.

Situation escalates when he now unwraps them but the price of USDc deops to 0.95$

which means his 1 Billion USDC will be now worth 0.95 Billion $

effectively making him suffer a Loss of 50000000$ or 50 Million$ which is huge