Aycozzynfada - Missing Cooldown require statement in deallocateForPartyB Function

[sherlock-admin3]

Aycozzynfada

Medium

Missing Cooldown require statement in deallocateForPartyB Function

Summary

The deallocateForPartyB function lacks a cooldown check, unlike similar functions like deallocate, which ensures a cooldown period between deallocations.

Root Cause

The root cause of the vulnerability is the absence of a cooldown check in the deallocateForPartyB function. This allows users to bypass the intended rate-limiting mechanism that is present in the deallocate function.

Attack Path

-A malicious user repeatedly calls the deallocateForPartyB function without any cooldown period. -The lack of a cooldown check allows the function to execute multiple times in quick succession. -This breaks crucial invariant by allowing repeated deallocations without waiting for the expected cooldown period to elapse

Impact

malicious users may exploit the lack of cooldown to manipulate intended behaviour of the protocol

PoC

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/Account/AccountFacetImpl.sol#L49-L64

deallocateForPartyB updates the block.timestamp for msg.sender in the last part of the code, but fails to implement the expected cooldown check before the function can be called again by the same caller.

    function deallocateForPartyB(uint256 amount, address partyA, SingleUpnlSig memory upnlSig) internal {
// No require statement to veto cooldown period
        AccountStorage.Layout storage accountLayout = AccountStorage.layout();
        require(accountLayout.partyBAllocatedBalances[msg.sender][partyA] >= amount, "AccountFacet: Insufficient allocated balance");
        LibMuonAccount.verifyPartyBUpnl(upnlSig, msg.sender, partyA);
        int256 availableBalance = LibAccount.partyBAvailableForQuote(upnlSig.upnl, msg.sender, partyA);
        require(availableBalance >= 0, "AccountFacet: Available balance is lower than zero");
        require(uint256(availableBalance) >= amount, "AccountFacet: Will be liquidatable");

        accountLayout.partyBAllocatedBalances[msg.sender][partyA] -= amount;
        accountLayout.balances[msg.sender] += amount;
        accountLayout.withdrawCooldown[msg.sender] = block.timestamp;
    }

N.B: withdrawFromReserveVault() also fails to implement expected check

Mitigation

Add a cooldown check in the deallocateForPartyB function similar to the one in the deallocate function. This will ensure a minimum time interval between consecutive deallocations.

function deallocateForPartyB(uint256 amount, address partyA, SingleUpnlSig memory upnlSig) internal {
        AccountStorage.Layout storage accountLayout = AccountStorage.layout();
    ++  require(
    ++      block.timestamp >= accountLayout.withdrawCooldown[msg.sender] + 
        ++          MAStorage.layout().deallocateDebounceTime,
            "AccountFacet: Too many deallocate in a short window"
        );
        require(accountLayout.partyBAllocatedBalances[msg.sender][partyA] >= amount, "AccountFacet: Insufficient allocated balance");
        LibMuonAccount.verifyPartyBUpnl(upnlSig, msg.sender, partyA);
        int256 availableBalance = LibAccount.partyBAvailableForQuote(upnlSig.upnl, msg.sender, partyA);
        require(availableBalance >= 0, "AccountFacet: Available balance is lower than zero");
        require(uint256(availableBalance) >= amount, "AccountFacet: Will be liquidatable");

        accountLayout.partyBAllocatedBalances[msg.sender][partyA] -= amount;
        accountLayout.balances[msg.sender] += amount;
        accountLayout.withdrawCooldown[msg.sender] = block.timestamp;
    }