bitsurfer - `pendingPrimaryWithdraw`, `pendingSecondaryWithdraw` is not cleared out on `_withdraw` resulting user can have instant withdrawal when `fastWithdraw` is disabled

[sherlock-admin]

bitsurfer

medium

pendingPrimaryWithdraw, pendingSecondaryWithdraw is not cleared out on _withdraw resulting user can have instant withdrawal when fastWithdraw is disabled

pendingPrimaryWithdraw, pendingSecondaryWithdraw is not cleared out on _withdraw resulting user can have instant withdrawal when fastWithdraw is disabled

Summary

pendingPrimaryWithdraw, pendingSecondaryWithdraw should be cleared out on _withdraw in case of user perform a requestWithdraw while the fastWithdraw is enabled to withdraw their asset.

Vulnerability Detail

Normally, withdrawing will require a certain delay or timelock, but if fastWithdraw is active, user can withdraw directly without need to wait withdraw timelock expired.

The issue here is that, when fastWithdraw is enabled but user do requestWithdraw, withdrawExecutionTimestamp is not cleared. Thus if the fastWithdraw is disabled later, user can execute the next withdrawal without any timelock.

This also makes, pendingPrimaryWithdraw and pendingSecondaryWithdraw are not cleared when user actually execute fastWithdraw.

those variables pendingPrimaryWithdraw, pendingSecondaryWithdraw and withdrawExecutionTimestamp should be cleared when the _withdrawal executed. Currently only by calling executeWithdraw the pendingPrimaryWithdraw and pendingSecondaryWithdraw are cleared to 0.

File: Funding.sol
69:     function requestWithdraw(
70:         Types.State storage state,
71:         address from,
72:         uint256 primaryAmount,
73:         uint256 secondaryAmount
74:     )
75:         external
76:     {
...
78:         state.pendingPrimaryWithdraw[msg.sender] = primaryAmount;
79:         state.pendingSecondaryWithdraw[msg.sender] = secondaryAmount;
80:         state.withdrawExecutionTimestamp[msg.sender] = block.timestamp + state.withdrawTimeLock;
...
82:     }
084:     function executeWithdraw(
085:         Types.State storage state,
086:         address from,
087:         address to,
088:         bool isInternal,
089:         bytes memory param
090:     )
091:         external
092:     {
...
097:         state.pendingPrimaryWithdraw[from] = 0;
098:         state.pendingSecondaryWithdraw[from] = 0;
099:         // No need to change withdrawExecutionTimestamp, because we set pending
100:         // withdraw amount to 0.
101:         _withdraw(state, msg.sender, from, to, primaryAmount, secondaryAmount, isInternal, param);
102:     }

Even though this seems not important or part of user mistakes, but this can potentially skip the timeout with this case:

1. user request withdrawal, pendingPrimaryWithdraw and pendingSecondaryWithdraw are not zero
2. before user executing executeWithdrawal, the protocol activate fastWithdrawal
3. user withdraw their asset (via fastWithdrawal), but pendingPrimaryWithdraw and pendingSecondaryWithdraw are still not zero
4. next time if protocol decided to disable fastWithdrawal, user still have an active pendingPrimaryWithdraw and pendingSecondaryWithdraw, and guarantee to instant executeWithdrawal

Also, the state of user will be wrong, for example executing executeWithdrawPrimaryAsset on DegenSubaccount will not be valid since it will fetch getCreditOf which is out of state.

File: DegenSubaccount.sol
92:     function executeWithdrawPrimaryAsset(address to, bool toInternal) external onlyOwner {
93:         (uint256 maxWithdrawValue, uint256 pendingPrimaryWithdraw) = getMaxWithdrawAmount(address(this));
94:         require(pendingPrimaryWithdraw <= maxWithdrawValue, "withdraw amount is too big");
95:         IDealer(dealer).executeWithdraw(address(this), to, toInternal, "");
96:     }

File: JOJOView.sol
40:     function getCreditOf(address trader)
41:         external
42:         view
43:         returns (
44:             int256 primaryCredit,
45:             uint256 secondaryCredit,
46:             uint256 pendingPrimaryWithdraw,
47:             uint256 pendingSecondaryWithdraw,
48:             uint256 executionTimestamp
49:         )
50:     {
51:         primaryCredit = state.primaryCredit[trader];
52:         secondaryCredit = state.secondaryCredit[trader];
53:         pendingPrimaryWithdraw = state.pendingPrimaryWithdraw[trader];
54:         pendingSecondaryWithdraw = state.pendingSecondaryWithdraw[trader];
55:         executionTimestamp = state.withdrawExecutionTimestamp[trader];
56:     }

Impact

unsynced state between primaryCredit and pendingPrimaryWithdraw, and also user can execute instant withdrawal even though the fastWithdrawal is disabled

Code Snippet

https://github.com/sherlock-audit/2023-12-jojo-exchange-update/blob/main/smart-contract-EVM/src/libraries/Funding.sol#L78-L80

Tool used

Manual Review

Recommendation

Consider to clear out the pendingPrimaryWithdraw, pendingSecondaryWithdraw, withdrawExecutionTimestamp on _withdraw function

[sherlock-admin2]

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

takarez commented:

invalid because { Since the fastWithdraw is a governance job to activate; i consider this invalid vecause they will makesure that there is no active requestWithdraw}

[nevillehuang]

Invalid, if user wants to inititate a time delayed request for withdrawal when fast withdrawal is active, sure he can do that, no issue here (I see no point in doing that). Additionally, initiating fast withdrawal is an admin gated mechanism.