PNS - Improper Implementation of ReentrancyGuard

[sherlock-admin3]

PNS

Medium

Improper Implementation of ReentrancyGuard

Summary

Some facets in the project use the ReentrancyGuard from OpenZeppelin, which utilizes a state variable to store its status. This could complicate future upgrades. The project is built using the Diamond pattern, which operates as a proxy. The entry point is the Diamond.sol contract (proxy), which serves as state storage for the facets' local variables.

Vulnerability Detail

The issue arises if a newly added facet has a state variable that can overwrite the ReentrancyGuard status (stored in slot 0). The project uses the DiamondStorage pattern and namespaces to structure stored data and avoid collisions. Each facet has its own storage and namespace.

However, ReentrancyGuard does not fit well with the project's storage management assumptions.

Additionally, the project has an AppStorage contract shared across the application, fixed at slot 0, which exacerbates the situation's seriousness.

For example, in OrderFacet.sol:batchCreateOrderRequest(), the nonReentrant modifier uses a local state variable, and AppConfig uses AppStorage, loaded from slot 0. This is mitigated only because AppStorage uses a structure containing only mappings, which have dynamically allocated slots.

File: contracts/facets/OrderFacet.sol
51:     function batchCreateOrderRequest(
52:         PlaceOrderParams[] calldata params
53:     ) external payable override nonReentrant {
54:         address account = msg.sender;
55:         Account.Props storage accountProps = Account.loadOrCreate(account);
56:         uint256 totalExecutionFee;
57:         AppConfig.ChainConfig memory chainConfig = AppConfig.getChainConfig(); //audit

Impact

The reentrancy guard status variable can be overwritten by other facets' local state variables, causing reentrancy protection to fail. This can lead to critical vulnerabilities in the system.

Code Snippet

• OrderFacet.sol
• AppStorage.sol
• OrderFacet.sol

Tool used

Manual Review

Recommendation

Follow the guidelines from the Diamond pattern creator and avoid using local state variables. Instead, store everything in AppStorage.

Here is a reference for a safe implementation:

• Beanstalk ReentrancyGuard

Consider implementing a custom ReentrancyGuard that stores its status in AppStorage. This would ensure the reentrancy status variable does not collide with other state variables from different facets, maintaining the integrity and security of the contract.

Duplicate of #194

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/0xCedar/elfi-perp-contracts/pull/64

[nevillehuang]

Invalid, speculation of future upgrades by protocol.

[pronobis4]

Escalate Escalation reported after a discussion on discord

More information about storage management in diamond: https://eip2535diamonds.substack.com/p/diamond-upgrades

[sherlock-admin3]

Escalate Escalation reported after a discussion on discord

More information about storage management in diamond: https://eip2535diamonds.substack.com/p/diamond-upgrades

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.

[WangSecurity]

To clarify, this won't pose any issues in the current version of the protocol, but will cause issues when other facets are added?

[pronobis4]

Yes

[WangSecurity]

I agree it's a valid issue, for more explanation, please refer to this comment.

But, I think this issue is indeed a duplicate of #194. I see that 194 is about the protocol in general, while this one about ReentrancyGuard, but I believe this report also fits the root cause of 194:

The project uses a structured storage scheme, allowing data in the form of structures to be appropriately linked with facets, theoretically enabling easy updates. However, a problem may arise in future updates because some of these structures contain nested structures that cannot be expanded without "corrupting" the data stored after them in the parent structure

I admit I may be missing something here, but the initial decision is to accept the escalation and duplicate this issue with #194 as a valid medium. Please correct me, if I'm wrong.

[WangSecurity]

Result: Medium Duplicate of #194

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• pronobis4: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.