delegatecall risks of clash due to structure storage

[GalloDaSballo]

Happy Saturday Paul, I was checking your code as I have worked on something similar, and noticed that your contracts are delegatecalling into an arbitrary target.

https://github.com/PaulRBerg/prb-proxy/blob/af820a73eb8796f0b45382751c45b11a7c0d3ebc/src/PRBProxy.sol#LL100C31-L100C48

I assume this is to allow the extension of logic via delegatecall

While the fallback is meant to only work with plugins, which will inherit the same Storage Strcuture, the delegatecall in execute looks unsafe, and has a very high likelihood of resulting in a storage clash.

From my research, it would be best to use a pseudo-random storage slot for all of the storage (just put all the values into a struct), at a pseudo-random location, which is basically what the Diamond Proxy Pattern does.

Hoping this helps

[PaulRBerg]

Hello Alex, thank you for taking the time to flag this issue.

I assume this is to allow the extension of logic via delegatecall

yes

While the fallback is meant to only work with plugins

The same function _safeDelegateCall is called in execute, so your feedback applies to both plugins (reactive contracts) and targets (proactive contracts).

it would be best to use a pseudo-random storage slot for all of the storage (just put all the values into a struct)

Interesting idea. I can see the benefit, although I need to weigh the trade-offs, e.g., increased gas cost and more complicated API.

pseudo-random location

Could you please share an example of how this is done in practice?

[GalloDaSballo]

pseudo-random location
Could you please share an example of how this is done in practice?

This is from the Diamond Standard Implementation, I'd be happy to share what I've written once is made open-source

1. Define Pseudo-random location (either use Diamond location or Proxies one)
   https://github.com/mudgen/diamond-3/blob/08e3b995b40b3eb8d54ac12f5e918fc9554161e9/contracts/libraries/LibDiamond.sol#L13

2. All storage is inside of a struct (since it needs to be offset by the random location)
   https://github.com/mudgen/diamond-3/blob/08e3b995b40b3eb8d54ac12f5e918fc9554161e9/contracts/libraries/LibDiamond.sol#L25

3. Extra overhead (mostly code, as you are always computing and getting a pointer for any SLOAD), is the function to fetch the storageLocation
   https://github.com/mudgen/diamond-3/blob/08e3b995b40b3eb8d54ac12f5e918fc9554161e9/contracts/libraries/LibDiamond.sol#L40

[PaulRBerg]

Thanks a lot, I'll have a look

[PaulRBerg]

This is no longer a problem since the latest PRs:

• refactor: remove "minGasReserve" #114
• refactor: storage redesign #120

I have completely removed the proxy's storage - the plugins and the permissions are stored in the PRBProxyRegistry contract now.