Fix unauthorized access #118
Conversation
| _msgSender() == _entryPoint | ||
| ); | ||
| _; | ||
| } |
There was a problem hiding this comment.
Should the virtual function _requireFromEntryPoint(...) in BaseAccount be overridden (to use _msgSender())
There was a problem hiding this comment.
Yeah probably. I'd suggest we make something like Safe4337Base we can inherit from, which uses BaseAccount and HandlerContext and implement this override.
I'm wary of inheritance in general though. I've always avoided it. However, it seems popular in the smart contracts we're working with, so I'm inclined to lean into it for now. Wdyt?
| @@ -50,7 +52,7 @@ contract SafeBlsPlugin is BaseAccount { | |||
| address to, | |||
| uint256 value, | |||
| bytes calldata data | |||
| ) external payable { | |||
| ) external payable fromThisOrEntryPoint { | |||
There was a problem hiding this comment.
Why does execTransaction need to allow this? (The safe can call its function execTransactionFromModule directly if needed)
There was a problem hiding this comment.
Good point. I think I was just a bit hazy on that when I originally wrote this code. Will update.
| @@ -50,7 +52,7 @@ contract SafeBlsPlugin is BaseAccount { | |||
| address to, | |||
| uint256 value, | |||
| bytes calldata data | |||
| ) external payable { | |||
| ) external payable fromThisOrEntryPoint { | |||
| address payable safeAddress = payable(msg.sender); | |||
There was a problem hiding this comment.
Is msg.sender always going to be the safe?
There was a problem hiding this comment.
Hmm I'm not sure. I think yes if the function is always executed via safe's module system. The msg.sender here is not part of the changes in this PR, but it looks wrong to me too.
I think the original source of using msg.sender here is safe's 4337 example. I opened a PR over there to change it to use this, curious to see what they say: safe-global/safe-smart-account#682.
Resolves #89 by using
HandlerContextto access_msgSender()and check that it'sthisor the entry point.