Extend fallback handler documentation

[mmv08]

The FallbackManager contract forwards all calls to the handler contract via the CALL opcode. Therefore in the next call frame the msg.sender variable equals the Safe address. There's potential room for mistakes which has to be clear in the documentation.

Example case:

• Fallback handler set to a token contract, then anyone could freely interact via this token contract on Safe's behalf

Proposed solution

Add a warning in the contract documentation

[MicahZoltu]

It would also be helpful to include an explanation why call opcode is used instead of delegatecall. delegatecall feels more appropriate here, and is the behavior I would expect for something like this. A sentence or two explaining why call is used would go a long way to helping future readers (and me) understand.

[mmv08]

It would also be helpful to include an explanation why call opcode is used instead of delegatecall. delegatecall feels more appropriate here, and is the behavior I would expect for something like this. A sentence or two explaining why call is used would go a long way to helping future readers (and me) understand.

I was not there when the original decision was made. Still, for me, delegatecall is required to extend the logic of the original contract, and that's already possible through modules or the core transaction execution flow. Also, since delegatecall is a massive security risk, I think leaving them to modules and core execution flow is better because they're opt-in features.

[MicahZoltu]

You cannot extend the original contract in the same way with modules as you can with fallback handlers. With a fallback handler, you can add the ability to have the contract fulfill new interface requirements (add callbacks) like ERC-165 or ERC-777. With modules, you can add new ways of having your safe take actions (besides just executing things).

I agree delegatecall is far more risky! But the same logic applies to modules, which already support delegate call. STATICCALL limitation would be relatively safe, but I feel like once you are giving something CALL ability, the step up to DELEGATECALL isn't a particularly big one as CALL lets you liquidate every asset the user owns except ETH.

[nlordell]

but I feel like once you are giving something CALL ability, the step up to DELEGATECALL isn't a particularly big one as CALL lets you liquidate every asset the user owns except ETH.

I’m not sure I follow... In particular, the Safe contract is CALL-ing the fallback handler, but (unless enabled by another mechanism such as modules) the fallback handler does not have the permissions to CALL on behalf of the Safe. So I don’t think a fallback handler can liquidate Safe funds as is (unless I’m missing something embarrassedly obvious).

[MicahZoltu]

Ah, I think you are right, my mistake! I still would like to see the ability to add DELEGATECALL fallback handlers, but the attack surface area is relatively small. I would also like to see the ability to install STATICCALL fallback handlers, so you could be very confident the installed handler couldn't do anything bad.