PNS - `AccountFacet.internalTransfer:81` Allows Bypassing Accounting Pause and Allocating Funds

[sherlock-admin4]

PNS

Medium

AccountFacet.internalTransfer:81 Allows Bypassing Accounting Pause and Allocating Funds

Summary

The internalTransfer function lacks the whenNotAccountingPaused modifier and does not verify that the transferred funds are directed to an account different from the sender’s.

Vulnerability Detail

According to the documentation:

This method allows a user to send their deposits to another user’s allocated balance. The receiver address cannot be a partyB and also should not be in a liquidated state.

However, the function does not check if msg.sender is different from the specified user. This oversight allows a user to transfer funds to themselves, thereby bypassing the whenNotAccountingPaused modifier, which might be blocking the allocate function at that moment.

// File: protocol-core/contracts/facets/Account/AccountFacet.sol
   79:  function internalTransfer(address user, uint256 amount) external whenNotInternalTransferPaused notPartyB userNotPartyB(user) notSuspended(msg.sender) notLiquidatedPartyA(user){

Impact

This is a critical issue in a system where such a bypass could lead to unregulated fund allocations, violating the intended accounting pause.

Code Snippet

AccountFacet.sol#L79

Tool used

Manual Review

Recommendation

Add a check to ensure that msg.sender is different from the specified user and apply the whenNotAccountingPaused modifier to the internalTransfer function to prevent bypassing the accounting pause.

[sherlock-admin3]

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

Hash01011122 commented:

Invalid, No attack vector or attack path mentioned in issue

[ABDuullahi]

Escalate

This should be a dupp of issue 11

The attack path is that of the bypassing of the accounting pause

[sherlock-admin3]

Escalate

This should be a dupp of issue 11

The attack path is that of the bypassing of the accounting pause

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.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/SYMM-IO/protocol-core/pull/47

[WangSecurity]

The problem with that report is the impact, "unregulated fund allocations, violating the intended accounting pause". Hence, this report doesn't show a Medium severity impact and, hence, cannot qualify to be a duplicate with new duplication standards.

Therefore, I believe the Lead Judge made the correct decision and this report has to remain excluded. Planning to reject the escalation.

[ABDuullahi]

From the main issue, the impacts mentioned are two and i'd consider them to be same, because its all about the transfer of the funds by bypassing the accounting pause.

There is an issue, error, or bug in certain areas (e.g., accounting) of the system. Thus, the funds transfer should be halted to prevent further errors from accumulating and to prevent users from suffering further losses due to this issue.

I believe that to be mentioned here also and thats the

such a bypass could lead to unregulated fund allocations, violating the intended accounting pause.

which if we translate, should be all about the transfer of funds without limitation.

And also, the attack path was mentioned here

This oversight allows a user to transfer funds to themselves, thereby bypassing the whenNotAccountingPaused modifier, which might be blocking the allocate function at that moment.

Except if im missing something, i believe this to be a dupp of issue 11 @WangSecurity

[WangSecurity]

From the main https://github.com/sherlock-audit/2024-06-symmetrical-update-2-judging/issues/11, the impacts mentioned are two and i'd consider them to be same, because its all about the transfer of the funds by bypassing the accounting pause.

This is the root cause and not an impact. Just bypassing the accounting pause still needs explanation how it impacts the system and the users.

I believe that to be mentioned here also and thats the

such a bypass could lead to unregulated fund allocations, violating the intended accounting pause.

which if we translate, should be all about the transfer of funds without limitation.

I still don't see how it qualifies for medium severity impact, it doesn't explain the loss of funds, while #11 does. Hence, this report is solely a recommendation, rather than an issue.

And also, the attack path was mentioned here

I see the attack, but the new duplication rules require every report to show the medium impact, which I believe is missing here.

The decision remains the same, planning to reject the escalation and leave the issue as it is.

[ABDuullahi]

hey @WangSecurity

i believe the issue did in fact have a medium severity impact, which is according to the rule to be either loss of funds and/or breaking a core functionality

heres also the overall reason why i believe it to be a dupp of issue 11

the rules says that all three requirements (out of 4) must be met in order to qualify

Identify the root cause

This issue identified the root cause > the function does not check if msg.sender is different from the specified user

Identify at least a Medium impact

The medium impact here is breaking core function which was mentioned here > violating the intended accounting pause

Identify a valid attack path or vulnerability path

The attack path also, which is the > This oversight allows a user to transfer funds to themselves, thereby bypassing the whenNotAccountingPaused modifier

The issue seems short but i believe it has all thats needed( as per the rule) to be a dupp of issue 11

[WangSecurity]

Firstly, I agree it identifies the root cause and shows an attack path. To save the time for each side, let's talk solely about impact, cause it's the problem here.

The medium impact here is breaking core function which was mentioned here > violating the intended accounting pause

Secondly, the medium severity is not just "breaks core function", it's the following:

Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

Thirdly, the report doesn't show why sending funds to yourself would cause an issue here and how it disrupts the accounting. Moreover, just bypassing the pause doesn't necessarily mean it's an issue, the report doesn't explain how it breaks the contract behaviour or causes loss of funds. The impact is very vague here and doesn't have sufficient proof it causes any issue for the protocol.

Hence, I believe this report should remain excluded. The decision is the same, reject the escalation and leave the issue as it is.

[pronobis4]

The reason for this report was mainly the inconsistency of the system. When the system is paused, the user cannot allocate funds, do what the allocate function does (transfer from balances to allocatedBalances), but the internalTransfer function allows to bypass pause and perform such an operation. The consequences depend on why the system was paused, which does not always have to mean a loss of funds, but it can, which is why it was reported.

The main issue also does not describe a specific impact, we do not know the specific reasons why someone stops accounting, we can only guess why.

[WangSecurity]

I see that the main report also doesn't have a very specific explanation of how it causes a loss of funds, but it at least gives two scenarios where it can lead to such an impact. But, this report doesn't and only gives vague "unregulated fund allocations", without explaining how it harms anyone.

The decision remains the same, planning to reject the escalation and leave the issue as it is.

[pronobis4]

Ok @WangSecurity, so the indicated error is correct, but the entire issue will be rejected due to the poor quality of the submission? If this is the case, I understand, there is nothing more to add here.

Thanks @ABDuullahi for pointing this out :handshake:

[WangSecurity]

Ok @WangSecurity, so the indicated error is correct, but the entire issue will be rejected due to the poor quality of the submission?

Not the poor quality, but the impact, which doesn't qualify for medium severity.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ABDUullahi: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.