Missing modifiers on functions in AccountFacet

Summary

There are missing modifiers in the functions of AccountFacet contract, that enable users to interact with them even when their functionality is paused.

Vulnerability Detail

partyB functions such as depositForPartyB, and allocateForPartyB use the modifierwhenNotPartyBActionsPaused, which pauses their functionality when partyB actions are paused. It's expected that partyA's functions like deposit and allocate would have whenNotPartyAActionsPaused. But, they don't have the modifier.

Similarly, whenNotAccountingPaused is not used for partyB functions like allocateForPartyB, depositForPartyB.

Impact

Not using the right modifiers would allow users to keep interacting with these functions even when their functionality is supposed to be paused.

Code Snippet

In AccountFacet, the functions: deposit, allocate, allocateForPartyB, depositForPartyB.

Tool used

Manual Review

Recommendation

Please use the appropriate modifiers for the various functions in AccountFacet contract.

Duplicate of #268

NishithPat commented 1 year ago

Escalate

This can be a duplicate of https://github.com/sherlock-audit/2023-06-symmetrical-judging/issues/268, considering that the impact is similar. Basically, allowing users to use the functionality even when it's supposed to be paused. Even if it's not a duplicate, this issue should be valid.

In my issue, I mention this particular line:

Similarly, whenNotAccountingPaused is not used for partyB functions like allocateForPartyB, depositForPartyB.

When accounting is paused one would expect calculations like the following to be paused as well:

AccountStorage.layout().balances[user] += amountWith18Decimals;

But, this calculation can be bypassed when accounting is paused in certain situations. I will try to explain my issue in a similar way that this issue did: https://github.com/sherlock-audit/2023-06-symmetrical-judging/issues/268

Consider a party that happens to be both partyA and partyB. Let's say the accounting has been paused, so partyA is not able to deposit using the deposit function because of the whenNotAccountingPaused modifier. But, they will still be able to deposit using depositForPartyB function as partyB, because it lacks whenNotAccountingPaused modifier. Thereby allowing a party to deposit and the accounting to be updated, even when it's not intended by the protocol.

whenNotAccountingPaused is defined like this:

modifier whenNotAccountingPaused() {
    require(!GlobalAppStorage.layout().globalPaused, "Pausable: Global paused");
    require(!GlobalAppStorage.layout().accountingPaused, "Pausable: Accounting paused");
    _;
}

So, when accounting is supposed to be paused and user balances should not be updated, not having whenNotAccountingPaused on functions like depositForPartyB breaks the use of pausing accounting. This is a valid medium as it breaks the intended functionality of the smart contract.

Also, this is what the sponsor had to say about this issue:

Check the answer to the third point.

sherlock-admin2 commented 1 year ago

Escalate

This can be a duplicate of https://github.com/sherlock-audit/2023-06-symmetrical-judging/issues/268, considering that the impact is similar. Basically, allowing users to use the functionality even when it's supposed to be paused. Even if it's not a duplicate, this issue should be valid.

In my issue, I mention this particular line:

Similarly, whenNotAccountingPaused is not used for partyB functions like allocateForPartyB, depositForPartyB.

When accounting is paused one would expect calculations like the following to be paused as well:

AccountStorage.layout().balances[user] += amountWith18Decimals;

But, this calculation can be bypassed when accounting is paused in certain situations. I will try to explain my issue in a similar way that this issue did: https://github.com/sherlock-audit/2023-06-symmetrical-judging/issues/268

Consider a party that happens to be both partyA and partyB. Let's say the accounting has been paused, so partyA is not able to deposit using the deposit function because of the whenNotAccountingPaused modifier. But, they will still be able to deposit using depositForPartyB function as partyB, because it lacks whenNotAccountingPaused modifier. Thereby allowing a party to deposit and the accounting to be updated, even when it's not intended by the protocol.

whenNotAccountingPaused is defined like this:
modifier whenNotAccountingPaused() {
    require(!GlobalAppStorage.layout().globalPaused, "Pausable: Global paused");
    require(!GlobalAppStorage.layout().accountingPaused, "Pausable: Accounting paused");
    _;
}
So, when accounting is supposed to be paused and user balances should not be updated, not having whenNotAccountingPaused on functions like depositForPartyB breaks the use of pausing accounting. This is a valid medium as it breaks the intended functionality of the smart contract.

Also, this is what the sponsor had to say about this issue:

Check the answer to the third point.

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.

NishithPat commented 1 year ago

There is another important point that should be noted. The functions withdraw and withdrawTo have the whenNotAccountingPaused modifier, but allocateForPartyB, depositForPartyB don't. That means if the protocol is ever under attack and if they want all accounting to be paused, users are still allowed to deposit, and allocate if they happen to be partyB (as they don't have the whenNotAccountingPaused modifier) but cannot withdraw because withdraw functions are paused (as they have the whenNotAccountingPaused modifier). That means if unaware users deposit in such a situation, they might not be able to withdraw their deposited amount because of accounting being paused.