giraffe - Users receive less than expected due to lack of slippage and deadline controls for requestWithdraw

[sherlock-admin]

giraffe

medium

Users receive less than expected due to lack of slippage and deadline controls for requestWithdraw

Summary

In FundingRateArbitrage, withdrawal is done in two steps: user submits requestWithdraw() and wait for owner to permitWithdrawRequests(). Due to the time lag between the two steps, a user may receive much less USDC than expected, and is also unable to cancel his request (could wait for an arbitrary amount of time for withdrawal to complete).

Vulnerability Detail

Based on code comments, there could be a time lag of 24 hours between a request and permitted withdrawal. There are several issues at hand:

1. User gets back less USDC than expected. getIndex() returns an index which is the ratio between the net value of the system and the total balance of USDC earned by liquidity providers. This index helps determine the amount of USDC that corresponds to the user's JUSD balance at the current state of the system. If index changes drastically between request and permit, the user could get back a lot less USDC than expected. Due to reasons mentioned below, this time lag could extend beyond 24 hours allowing the index to change further due to market conditions.

   function requestWithdraw(uint256 repayJUSDAmount) external returns (uint256 withdrawEarnUSDCAmount) {
   	...
   	uint256 index = getIndex();
       uint256 lockedEarnUSDCAmount = jusdOutside[msg.sender].decimalDiv(index);
       ...
   }

2. User's request cannot be fulfilled and is stuck. If index changes significantly, there is a chance that amount < fees and the permit will revert. In another scenario, if a user is blacklisted from USDC, the transfer of USDC will fail and the permit will revert. At this point, user would have transferred JUSD out but is unable to receive USDC back until the request is permitted.

   function permitWithdrawRequests(uint256[] memory requestIDList) external onlyOwner {
   	...
   	//@audit index is recalculated here
   	uint256 USDCAmount = request.earnUSDCAmount.decimalMul(index);
   	require(USDCAmount >= withdrawSettleFee, "USDCAmount need to bigger than withdrawSettleFee");
   	...
   
   	//@audit If user is blacklisted, this would fail
   	IERC20(usdc).transfer(request.user, USDCAmount - feeAmount);
   	...

3. There could be a variety of other reasons why a user might want to cancel his request: e.g. changing market conditions, desire to use JUSD elsewhere etc. However, once the request is made there are no avenues to rescind it and a user could technically wait indefinitely for the Owner to fulfil a withdraw request.

Impact

User may get back less USDC than expected and/or could wait indefinitely for the request to be permitted, with no avenue to rescind the request.

Code Snippet

https://github.com/sherlock-audit/2023-12-jojo-exchange-update/blob/main/smart-contract-EVM/src/FundingRateArbitrage.sol#L282
https://github.com/sherlock-audit/2023-12-jojo-exchange-update/blob/main/smart-contract-EVM/src/FundingRateArbitrage.sol#L304

Tool used

Manual Review

Recommendation

1. Allow for users to input a slippage parameter for minimum USDC amount to be received.
2. Allow users to set a deadline for request to be fulfilled, after which the request is cancelled.
3. Allow users to cancel withdraw requests.

Duplicate of #35

[sherlock-admin2]

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

takarez commented:

valid because { This is a valid medium findings; the watson explained how user might recieve less amount than his intended amount; this is valid and more recommendation will be to use the same index used in the requestWithdraw}

[giraffe0x]

Escalate.

Disagree that it is a duplicate of #35 based on my recommendations for a slippage parameter and deadline.

My recommendation would fix #35 but it does more than fix that unique scenario. For the same reasons that slippage and deadline are necessary in swaps (common medium finding), a user should expect to get back an amount + within a timeline that is reasonable. The current design could see a user getting back exceptionally less and wait indefinitely for the withdrawal to complete.

[sherlock-admin]

Escalate.

Disagree that it is a duplicate of #35 based on my recommendations for a slippage parameter and deadline.

My recommendation would fix #35 but it does more than fix that unique scenario. For the same reasons that slippage and deadline are necessary in swaps (common medium finding), a user should expect to get back an amount + within a timeline that is reasonable. The current design could see a user getting back exceptionally less and wait indefinitely for the withdrawal to complete.

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]

@giraffe0x This issue has the exact same root cause as #35. Just because you introduce a deadline check doesn't mean it suffice a separation, even more so when recently, deadline checks has been deemed as not sufficient to be deemed as medium severity anymore in sherlock, given it is users responsibility to set high enough gas to execute the request and slippage alone would be sufficient to resolve this issue. Additionally, to my knowledge, the loss is only on paper, given JUSD can always be retained with the same value as the collateral initially deposited, but during the time of transaction, if index price changes, that is another topic up for debate

Examples:

sherlock-audit/2023-06-tokemak-judging#886
sherlock-audit/2023-10-real-wagmi-judging#51 (comment)

[Evert0x]

Planning to reject escalation and keep issue state as is, because root cause in both reports is the same.

[Czar102]

Result:
Invalid
Duplicate of #35

Please note that the severity may change due to an active escalation on #35.

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• giraffe0x: rejected