xiaoming90 - Two restrictions ( `deallocateDebounceTime` and `deallocateCooldown`) can be bypassed

[sherlock-admin2]

xiaoming90

Medium

Two restrictions ( deallocateDebounceTime and deallocateCooldown) can be bypassed

Summary

The two restrictions ( deallocateDebounceTime and deallocateCooldown) are important security properties to enforce within the protocol. However, they can be bypassed. In the event of an attack, malicious users could bypass these restrictions to quickly deallocate multiple times within a short period of time, potentially draining the protocol.

Vulnerability Detail

Per the documentation, a new condition to enforce a debounce time between deallocation requests to prevent too frequent deallocations has been added at Line 52 below.

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/Account/AccountFacetImpl.sol#L52

File: AccountFacetImpl.sol
49:     function deallocate(uint256 amount, SingleUpnlSig memory upnlSig) internal {
50:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
51:         require(
52:             block.timestamp >= accountLayout.withdrawCooldown[msg.sender] + MAStorage.layout().deallocateDebounceTime,
53:             "AccountFacet: Too many deallocate in a short window"
54:         );
55:         require(accountLayout.allocatedBalances[msg.sender] >= amount, "AccountFacet: Insufficient allocated Balance");
56:         LibMuon.verifyPartyAUpnl(upnlSig, msg.sender);
57:         int256 availableBalance = LibAccount.partyAAvailableForQuote(upnlSig.upnl, msg.sender);
58:         require(availableBalance >= 0, "AccountFacet: Available balance is lower than zero");
59:         require(uint256(availableBalance) >= amount, "AccountFacet: partyA will be liquidatable");
60: 
61:         accountLayout.allocatedBalances[msg.sender] -= amount;
62:         accountLayout.balances[msg.sender] += amount;
63:         accountLayout.withdrawCooldown[msg.sender] = block.timestamp;
64:     }

However, it was found that there is a workaround to bypass this restriction. Assume that Alice (PartyA) wants to deallocate some funds and withdraw them to her wallet address. However, Alice is restricted by the deallocateDebounceTime as she has just deallocated a while back.

In addition, Alice is also restricted by the deallocateCooldown at Line 30 below, which prevents Alice from withdrawing the funds out of the protocol shortly after deallocating.

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/Account/AccountFacetImpl.sol#L30

File: AccountFacetImpl.sol
26:     function withdraw(address user, uint256 amount) internal {
27:         AccountStorage.Layout storage accountLayout = AccountStorage.layout();
28:         GlobalAppStorage.Layout storage appLayout = GlobalAppStorage.layout();
29:         require(
30:             block.timestamp >= accountLayout.withdrawCooldown[msg.sender] + MAStorage.layout().deallocateCooldown,
31:             "AccountFacet: Cooldown hasn't reached"
32:         );
33:         uint256 amountWith18Decimals = (amount * 1e18) / (10 ** IERC20Metadata(appLayout.collateral).decimals());
34:         accountLayout.balances[msg.sender] -= amountWith18Decimals;
35:         IERC20(appLayout.collateral).safeTransfer(user, amount);
36:     }

Alice could bypass these two restrictions ( deallocateDebounceTime and deallocateCooldown) via the following workaround:

• Alice colludes with a PartyB and an affiliator. Technically, PartyA is permissionless, so nothing prevents PartyB or affiliator from being a PartyA at the same time. Thus, it is possible that PartyB or affiliator also owns a PartyA account that allows them to exploit this issue.
• Assume that the trade fee is 5%. For each position opened, 5% of the notional value of the positions will be transferred to the affiliator's account, as shown in Line 102 below.
• Alice and PartyB open and close positions multiple times, causing the trade fee to be transferred to affiliator's account repeatedly
• Once the desired amount of fee has accumulated in affiliator's account, the affiliator could call the withdraw function to withdraw the funds immediately without any cooldown. This is because when the balance is accumulated to affiliator's account, the accountLayout.withdrawCooldown[user] was not updated.

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L99

File: PartyBFacetImpl.sol
078:    function openPosition(
..SNIP..
097:        if (quote.orderType == OrderType.LIMIT) {
098:            require(quote.quantity >= filledAmount && filledAmount > 0, "PartyBFacet: Invalid filledAmount");
099:            accountLayout.balances[GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate]] += (filledAmount * quote.requestedOpenPrice * quote.tradingFee) / 1e36;
100:        } else {
101:            require(quote.quantity == filledAmount, "PartyBFacet: Invalid filledAmount");
102:            accountLayout.balances[GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate]] += (filledAmount * quote.marketPrice * quote.tradingFee) / 1e36;
103:        }

Impact

The two restrictions ( deallocateDebounceTime and deallocateCooldown) are important security properties to enforce within the protocol. However, they can be bypassed. In the event of an attack, malicious users could bypass these restrictions to quickly deallocate multiple times within a short period of time, potentially draining the protocol.

Code Snippet

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/Account/AccountFacetImpl.sol#L52

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/Account/AccountFacetImpl.sol#L30

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/main/protocol-core/contracts/facets/PartyB/PartyBFacetImpl.sol#L99

Tool used

Manual Review

Recommendation

Whenever an account's balance is updated, the accountLayout.withdrawCooldown[user] should be updated to the current timestamp to ensure that no one can abuse this bypass the cooldown, and to ensure that the recipient is subjected to the relevant cooldown period.

if (quote.orderType == OrderType.LIMIT) {
  require(quote.quantity >= filledAmount && filledAmount > 0, "PartyBFacet: Invalid filledAmount");
  accountLayout.balances[GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate]] += (filledAmount * quote.requestedOpenPrice * quote.tradingFee) / 1e36;
} else {
  require(quote.quantity == filledAmount, "PartyBFacet: Invalid filledAmount");
  accountLayout.balances[GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate]] += (filledAmount * quote.marketPrice * quote.tradingFee) / 1e36;
}
+ accountLayout.withdrawCooldown[GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate]] = block.timestamp;

[sherlock-admin2]

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

Hash01011122 commented:

Low/Med vulnerability, Highly hypothetical scenario: Alice colludes with a PartyB and an affiliator. Technically, PartyA is permissionless, so nothing prevents PartyB or affiliator from being a PartyA at the same time. Thus, it is possible that PartyB or affiliator also owns a PartyA account that allows them to exploit this issue