PNS - Inconsistent Withdraw Event Emitted by `AccountFacet.internalTransfer:81`

[sherlock-admin2]

PNS

Medium

Inconsistent Withdraw Event Emitted by AccountFacet.internalTransfer:81

Summary

The internalTransfer function emits a Withdraw event with an inconsistent number of decimals compared to the withdraw and withdrawTo functions.

Vulnerability Detail

Functions that directly emit events like Deposit and Withdraw in the system do so with amounts in the correct collateral decimals as specified by their documentation.

// File: protocol-core/contracts/facets/Account/AccountFacet.sol
   32:  /// @param amount The precise amount of collateral to be withdrawn, specified in collateral decimals.
   33:  function withdraw(uint256 amount) external whenNotAccountingPaused notSuspended(msg.sender) {

However, the Withdraw event is also emitted by the internalTransfer function, which according to its documentation, takes an amount parameter with 18 decimals, which is then passed directly to the Withdraw event.

// File: protocol-core/contracts/facets/Account/AccountFacet.sol
   78:  /// @param amount The amount to transfer and allocate in 18 decimals.
   79:  function internalTransfer(address user, uint256 amount) external whenNotInternalTransferPaused notPartyB userNotPartyB(user) notSuspended(msg.sender) notLiquidatedPartyA(user){
   80:      AccountFacetImpl.internalTransfer(user, amount);
   81:      emit InternalTransfer(msg.sender, user, AccountStorage.layout().allocatedBalances[user], amount);
   82:      emit Withdraw(msg.sender, user, amount); // audit: should be in collateral decimals like original withdraw()
   83:      emit AllocatePartyA(user, amount, AccountStorage.layout().allocatedBalances[user]);
   84:      emit SharedEvents.BalanceChangePartyA(user, amount, SharedEvents.BalanceChangeType.ALLOCATE);
   85:  }

Impact

This inconsistency is problematic in a system that heavily relies on events (e.g. for tracking new requests by external systems that fulfill intents). Solidity does not inherently provide a straightforward way to distinguish which function emitted an event, which can lead to incorrect assessments by external systems, resulting in erroneous business decisions and lost of founds.

Docs: How To Implement Your Own Hedger For SYMMIO

Code Snippet

AccountFacet.transfer#L32-L33

AccountFacet.internalTransfer#L78-L85

Tool used

Manual Review

Recommendation

Convert the amount to be consistent with the collateral's decimals and emit the event with a standardized format.

[sherlock-admin2]

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

Hash01011122 commented:

Invalid

[pronobis4]

Escalate

Contest README:

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

Yes, report potential issues, including broken assumptions about function behavior, if they pose future integration risks. Key properties that should hold include correctness (accurate returns), security (resistant to manipulation), consistency (uniform behavior on-chain and off-chain), and reliability (functioning correctly under all conditions).

Additionally, the official documentation "How To Implement Your Own Hedger For SYMMIO" (4th paragraph) recommends using events:

The main connection between PartyA and PartyB is based on onchain contracts. Consequently, it is essential for each hedger to be able to monitor requests made by PartyA on the blockchain. This can be achieved by utilizing a subgraph or event-listener for example you can see this link SYMMIO-Subgraph which is a subgraph to SYMMIO on BSC net or using or for event listener using ethers-JS. Moreover, it is imperative for the hedgers to respond to these requests via onchain calls to the SYMMIO core contracts.

That's why I say that events are absolutely important in this system, because they are not just a simple log, but are an important part used in integrations with the system. Maybe withdraw() does not seem to be the most important (although it depends on the implemented integration), but at least it should always work the same and return consistent data, which in this case it does not do, forcing the integrator to perform additional operations of detecting and recalculating the issued amounts.

[sherlock-admin3]

Escalate

Contest README:

Should potential issues, like broken assumptions about function behavior, be reported if they could pose risks in future integrations, even if they might not be an issue in the context of the scope? If yes, can you elaborate on properties/invariants that should hold?

Yes, report potential issues, including broken assumptions about function behavior, if they pose future integration risks. Key properties that should hold include correctness (accurate returns), security (resistant to manipulation), consistency (uniform behavior on-chain and off-chain), and reliability (functioning correctly under all conditions).

Additionally, the official documentation "How To Implement Your Own Hedger For SYMMIO" (4th paragraph) recommends using events:

The main connection between PartyA and PartyB is based on onchain contracts. Consequently, it is essential for each hedger to be able to monitor requests made by PartyA on the blockchain. This can be achieved by utilizing a subgraph or event-listener for example you can see this link SYMMIO-Subgraph which is a subgraph to SYMMIO on BSC net or using or for event listener using ethers-JS. Moreover, it is imperative for the hedgers to respond to these requests via onchain calls to the SYMMIO core contracts.

That's why I say that events are absolutely important in this system, because they are not just a simple log, but are an important part used in integrations with the system. Maybe withdraw() does not seem to be the most important (although it depends on the implemented integration), but at least it should always work the same and return consistent data, which in this case it does not do, forcing the integrator to perform additional operations of detecting and recalculating the issued amounts.

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/54

[Hash01011122]

This is informational issue, on what basis do you think this should be considered as a valid medium @pronobis4

[pronobis4]

Because the sponsor has clearly stated that it cares about future integrations, that data consistency is important and that the entire system is based on external collaborations. Events in this system are not treated only as log information, but are used more as an external API on the basis of which integrations are built. Integrated systems/protocols must make decisions based on information from events in a very short time window. Therefore, data consistency in such systems is of particular value and the same event cannot emit data with different accuracy (decimals).

I argue that in these types of systems, events are a core functionality and can lead to the partner losing funds and therefore deserve a medium.

[MxAxM]

This is low/informational, doesn't impact the protocol

[WangSecurity]

a system that heavily relies on events (e.g. for tracking new requests by external systems that fulfill intents)

Could you clarify this part, is it documented or an assumption? As I see in the README Future Integrations question, it doesn't talk about events, but about the values returned in the functions. Hence, I believe event emissions do not fall under this category. Am I missing something?

[WangSecurity]

If No answer is provided, planning to reject the escalation and leave the issue as it is.

[pronobis4]

It all depends on what I take as the return on the function. In the direct/base case this will be the return defined by "returns" in solidity, but if the system uses events for integration, the values ​​emitted are also the return from the function.

But this is not the only case, because the sponsor also cares about "consistency (uniform behavior on-chain and off-chain), and reliability (functioning correctly under all conditions)". In my opinion this has also been broken.

Additionally, the fact that the system recommends events for integration is taken from the official documentation presented above.

[WangSecurity]

It all depends on what I take as the return on the function. In the direct/base case this will be the return defined by "returns" in solidity, but if the system uses events for integration, the values ​​emitted are also the return from the function.

I still believe that it means the actual return value and not the value in the event emission.

But this is not the only case, because the sponsor also cares about "consistency (uniform behavior on-chain and off-chain), and reliability (functioning correctly under all conditions)". In my opinion this has also been broken.

The problem is that the behaviour and functionality are not broken. The protocol functions correctly and successfully, but it's just the event emission that is incorrect. Hence, since no event emissions were mentioned in the README, the judging rules should apply. Therefore, event emission-related issues should be low severity.

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

[pronobis4]

I think there is nothing more to add, everything has been said, if that is the decision then so be it.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• pronobis4: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.