Modular proposal in the contracts

[luislucena16]

This is more of a proposal. It's a matter of preference :)... I think you could modularise the interface and tidy up a bit what's there, maybe errors, events and constants could have a separate contract for each one

PS: in case it grows over time!

[PaulRBerg]

Thanks for your suggestion, @luislucena16!

Just to clarify what you mean - do you want something like this for errors, and do the same for events and constants?

Related: Use libraries as namespaces for structs and enums

[luislucena16]

I can also propose that all the internal functions and the struct could go into a contract that is called Storage.

for example:

• Proxy1 has internal functions and struct
• Create Proxy1Storage and stores what it is entitled to

[PaulRBerg]

Actually, that's how it used to be, but I removed that design in this PR:

#120

[luislucena16]

I see it! any specific reason of the change?

[PaulRBerg]

@luislucena16 yep, see this audit report.

Plugins and permissions storage variables are unprotected · Issue #13 · cantinasec_review-sablier2.pdf

[luislucena16]

okay I see! I understand the point, some things I consider are these:

(i) the change was made because of the vulnerability of them being able to access mapping or variables that somehow end up being stored in the PRBProxyStorage, but I think it could have been handled as internal and had something like this:

• this is how you had it:

abstract contract PRBProxyStorage is IPRBProxyStorage {
address public override owner;
uint256 public override minGasReserve;
mapping(bytes4 method => IPRBProxyPlugin plugin) public plugins;
mapping(address envoy => mapping(address target => bool permission)) public permissions;

• so I thought:

abstract contract PRBProxy is PRBProxyStorage {
address internal override owner;
uint256 internal override minGasReserve;
mapping(bytes4 method => IPRBProxyPlugin plugin) internal plugins;
mapping(address envoy => mapping(address target => bool permission)) internal permissions;

the inheritance would be PRBProxy is PRBProxyStorage (removing the interface) and what contains PRBProxyStorage would be all internal.

ii) I think a validation could also be added here to ensure that the address is not a contract to avoid any malicious intent. A function written in assembly that focuses on that validation. What do you think?

contract PRBProxyRegistry {
function installPlugin(address owner, IPRBProxyPlugin plugin) external override {
if (proxies[owner] != msg.sender) { revert("Unauthorized"); }
// Impossible to check if `msg.sender` is the proxy itself or a malicious delegate-ca
  }
}