Redundant sender checks in CustomProxyAdmin.changeImplementation function

[coderabbitai]

In CustomProxyAdmin.sol, the changeImplementation function contains redundant sender checks:

if (msg.sender != bootstrapper) {
    revert Errors.CustomProxyAdminOnlyCalledFromBootstrapper();
}
if (msg.sender != address(proxy)) {
    revert Errors.CustomProxyAdminOnlyCalledFromProxy();
}

Since bootstrapper is intended to be the address of the proxy, checking both msg.sender != bootstrapper and msg.sender != address(proxy) is redundant. Verifying msg.sender == address(proxy) should suffice to prevent unauthorized upgrades.

This issue was identified in PR #102, see comment.

cc: @adu-web3

[MaxMustermann2]

I disagree with this issue. Verifying only msg.sender == address(proxy) will allow any proxy controlled by this CustomProxyAdmin to upgrade itself, even if it is not the Bootstrap contract. While the proxy and upgrade pattern is a good idea, playing with it this way is like playing with fire.

[adu-web3]

I understand that msg.sender == address(proxy) requires that the message caller must be the proxy itself, and then this custom proxy admin would call proxy.upgradeToAndCall to upgrade the logic contract. In this case, we may not need the state bootstrapper, and could even remove proxy from params by using msg.sender as proxy?

[MaxMustermann2]

I see your point, but I still believe keeping the bootstrapper check is important for security. Relying solely on msg.sender == address(proxy) could open the door for any proxy governed by this admin to upgrade itself, which is risky. The bootstrapper ensures that only the designated Bootstrap contract can make this call. Removing that check feels like compromising safety for convenience. Also, using msg.sender as proxy could work, but it seems like a workaround that trades explicitness for implicit logic, which isn’t always a good thing.

Thoughts?

[adu-web3]

make sense to keep bootstrapper, but passing in proxy and then checking that msg.sender == proxy is equal to proxy = msg.sender, so we might be able to remove the proxy param by using msg.sender as proxy. what do you think?

[MaxMustermann2]

Yes that seems reasonable. I understand what you're saying now, thanks.