xiaoming90 - Suspended users can receive funds

[sherlock-admin3]

xiaoming90

High

Suspended users can receive funds

Summary

Suspended users can receive funds from others. As a result, funds will be transferred to a suspended account, leading to funds being frozen. No user error is required for this issue to be triggered. This issue can happen under normal circumstances due to how blockchain transactions work. Refer to the "Impact" section for more details.

Vulnerability Detail

The AccountFacet.allocate function has a notSuspended(msg.sender) modifier to ensure that no collateral is being allocated to a suspended account.

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

File: AccountFacet.sol
46: 	/// @notice Allows Party A to allocate a specified amount of collateral. Allocated amounts are which user can actually trade on.
47: 	/// @param amount The precise amount of collateral to be allocated, specified in 18 decimals.
48: 	function allocate(uint256 amount) external whenNotAccountingPaused notSuspended(msg.sender) notLiquidatedPartyA(msg.sender) {
49: 		AccountFacetImpl.allocate(amount);
..SNIP..
52: 	}

However, malicious users can bypass this invariant or restriction by exploiting the newly implemented AccountFacet.internalTransfer function. This function only checks if the sender (msg.sender) is suspended but does not check if the recipient (user) is suspended. Thus, it is possible to allocate funds to a suspended user.

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

File: AccountFacet.sol
79: 	function internalTransfer(address user, uint256 amount) external whenNotInternalTransferPaused notPartyB userNotPartyB(user) notSuspended(msg.sender) notLiquidatedPartyA(user){
80: 		AccountFacetImpl.internalTransfer(user, amount);
..SNIP..
85: 	}

Impact

Funds will be transferred to a suspended account, leading to funds being frozen. No user error is required for this issue to be triggered. This issue can happen under normal circumstances due to how blockchain transactions work. Consider the following scenario:

1. Alice wants to allocate funds from her account to Bob's account
2. Alice did her due diligence. She used Etherscan or Foundry's cast to check to check that AccountStorage.layout().suspendedAddresses[Bob]is false (not suspended) before submitting the transaction.
3. Alice sees that Bob's is not suspended, so she submits an internalTransfer transaction to the mempool to allocate funds from her account to Bob's account.
4. The protocol admin submits a suspendedAddress transaction to suspend Bob's account shortly afterward
5. Both internalTransfer and suspendedAddress transactions reside in the mempool. Since suspendedAddress TX has a higher gas fee than internalTransfer TX, it will be executed first.
6. When internalTransfer TX gets executed, the funds will be allocated to a suspended Bob account.

Note that in Step 3, there is no way for Alice to know that the protocol admin is going to submit a suspendedAddress transaction after her.

Code Snippet

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

Tool used

Manual Review

Recommendation

The internal transfer should only proceed if the recipient has not been suspended. With this fix, Alice's internalTransfer TX will automatically revert to the earlier scenario.

- function internalTransfer(address user, uint256 amount) external whenNotInternalTransferPaused notPartyB userNotPartyB(user) notSuspended(msg.sender) notLiquidatedPartyA(user){
+ function internalTransfer(address user, uint256 amount) external whenNotInternalTransferPaused notPartyB userNotPartyB(user) notSuspended(msg.sender) notSuspended(user) notLiquidatedPartyA(user){
  AccountFacetImpl.internalTransfer(user, amount);

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
SYMM-IO/protocol-core#51

[xiaoming9090]

Escalate

This issue should be at least a Medium Risk due to its significant impact (Loss of assets) if the risk is realized.

As mentioned in the impact section of the report, it demonstrates that under certain conditions, it can lead to a loss of assets for innocent users. Following is the POC extracted from the report to demonstrate this:

Funds will be transferred to a suspended account, leading to funds being frozen. No user error is required for this issue to be triggered. This issue can happen under normal circumstances due to how blockchain transactions work. Consider the following scenario:

1. Alice wants to allocate funds from her account to Bob's account
2. Alice did her due diligence. She used Etherscan or Foundry's cast to check to check that AccountStorage.layout().suspendedAddresses[Bob]is false (not suspended) before submitting the transaction.
3. Alice sees that Bob's is not suspended, so she submits an internalTransfer transaction to the mempool to allocate funds from her account to Bob's account.
4. The protocol admin submits a suspendedAddress transaction to suspend Bob's account shortly afterward
5. Both internalTransfer and suspendedAddress transactions reside in the mempool. Since suspendedAddress TX has a higher gas fee than internalTransfer TX, it will be executed first.
6. When internalTransfer TX gets executed, the funds will be allocated to a suspended Bob account.

Note that in Step 3, there is no way for Alice to know that the protocol admin is going to submit a suspendedAddress transaction after her.

Per Sherlock’s judging rules (https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue)

V. How to identify a medium issue:
Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

Thus, this issue meets Sherlock's requirement of a Medium Risk, where it causes a loss of funds but requires certain external conditions or specific states.

[sherlock-admin3]

Escalate

This issue should be at least a Medium Risk due to its significant impact (Loss of assets) if the risk is realized.

As mentioned in the impact section of the report, it demonstrates that under certain conditions, it can lead to a loss of assets for innocent users. Following is the POC extracted from the report to demonstrate this:

Funds will be transferred to a suspended account, leading to funds being frozen. No user error is required for this issue to be triggered. This issue can happen under normal circumstances due to how blockchain transactions work. Consider the following scenario:

1. Alice wants to allocate funds from her account to Bob's account
2. Alice did her due diligence. She used Etherscan or Foundry's cast to check to check that AccountStorage.layout().suspendedAddresses[Bob]is false (not suspended) before submitting the transaction.
3. Alice sees that Bob's is not suspended, so she submits an internalTransfer transaction to the mempool to allocate funds from her account to Bob's account.
4. The protocol admin submits a suspendedAddress transaction to suspend Bob's account shortly afterward
5. Both internalTransfer and suspendedAddress transactions reside in the mempool. Since suspendedAddress TX has a higher gas fee than internalTransfer TX, it will be executed first.
6. When internalTransfer TX gets executed, the funds will be allocated to a suspended Bob account.

Note that in Step 3, there is no way for Alice to know that the protocol admin is going to submit a suspendedAddress transaction after her.

Per Sherlock’s judging rules (https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue)

V. How to identify a medium issue:
Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

Thus, this issue meets Sherlock's requirement of a Medium Risk, where it causes a loss of funds but requires certain external conditions or specific states.

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.

[MxAxM]

isn't it more a self-harm than a vulnerability, why should I transfer funds to a suspended account

[xiaoming9090]

isn't it more a self-harm than a vulnerability, why should I transfer funds to a suspended account

It will only be self-harm if Alice is already aware of Bob's account being suspended and she proceeds to transfer the funds, ignoring this fact.

As mentioned in the report and escalation comment, in Step 3 of POC, there is no way for Alice to know that the protocol admin is going to submit a suspendedAddress transaction after her due to how the blockchain operates. The POC highlighted that Alice had done her due diligence before transferring the funds to ensure Bob's account was not suspended (Refer to Step 2 of POC). However, the issue still occurred after her due diligence.

Thus, it is not self-harm, and the above POC presents a valid scenario.

[WangSecurity]

Very good catch, but I believe it should be invalid based on this rule about admin calls:

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

Hence, I believe the lead judge made the correct decision, planning to reject the escalation and leave the issue as it is.

[xiaoming9090]

Very good catch, but I believe it should be invalid based on this rule about admin calls:

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

Hence, I believe the lead judge made the correct decision, planning to reject the escalation and leave the issue as it is.

@WangSecurity

The judging rule "An admin action can break certain assumptions about the functioning of the code" is inadequate for use here. This rule has two parts:

1. Admin action - Correct. Requires admin to suspend a user to kick off the issue. Note that this is a normal operation.
2. break certain assumptions about the functioning of the code - Incorrect. I do not see any assumption about the functioning of the code being broken here. Thus, it does not make the requirement of this rule. The protocol's requirement is that no suspended users can receive funds. However, due to missing notSuspended modifier, suspended users can receive funds. This deviates from the protocol's requirement, and this is not an assumption. The bug is pretty straightforward. Also, the admin or user did not make any mistake or perform a malicious action here. They simply use the system as per usual, and yet the issue gets realized.

On a sidenote, the reason this particular judging rule has been enacted in the past is that in many past contests, many people have raised the issue that users will be unfairly liquidated after the protocol has been unpaused (Report 1, Report 2. Report 3, Report 4), which might involve breaking some sort of assumptions such those below in order for the issue to be realized or triggered:

• Admin always performs the actions in the correct sequence, and they always act in good faith.
• The price of the assets is not volatile, and assets are not illiquid in the market
• The liquidation mechanism will work perfectly and efficiently. Liquidators are active and will perform liquidation against accounts in a timely manner once the account reaches the liquidation threshold.

[WangSecurity]

The protocol's requirement is that no suspended users can receive funds.

As I understand this requirement is just how the code works and is not documented in README or code comments (or at least I'm missing it).

Let me explain how I see the issue:

1. If the user just calls internalTransfer and sends funds to the Suspended user, it would be a mistake. I see from the report, that you also agree with that.

So the only valid scenario is when the admin submits the transaction at almost the same time as the user, but the admin's call goes through earlier, that's why it wouldn't be a mistake in that case.

But, since it requires the admin to make the call (without any malicious intent or an error) that leads to Alice losing funds, I believe it falls down this rule.

Hope that answers your questions. The decision remains the same, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.