search

brinktrade / brink-core

Core smart contracts for Brink accounts
GNU General Public License v3.0
13 stars 7 forks source link

Audit Issue 26: Prevent direct calls to account.sol #25

Closed Kyrrui closed 3 years ago

Kyrrui commented 3 years ago

Warning: This caused gas to go up by a couple thousand for each type of call, not sure it's worth the protection it gives us

mikec commented 3 years ago

@Kyrrui proxyOwner will be 0x0 on the deployed account.sol contract, so because of the proxyOwner checks, the calls can't be made directly, since msg.sender can't be 0x0, and I don't think that there's any way for ecrecover to return 0x0. I think externalCall(), delegateCall(), and metaDelegateCall() are implicitly protected from direct calls because of this.

However, I think the metaDelegateCall_EIP1271() https://github.com/brinktrade/brink-core/blob/master/contracts/Account/Account.sol#L97 would make it possible to call directly to Account.sol and execute arbitrary calldata, which would allow selfdestruct .. which would break all Proxy contracts. So this might actually be a critical vulnerability.

mikec commented 3 years ago

actually I think https://github.com/brinktrade/brink-core/blob/master/contracts/Account/EIP1271Validator.sol#L19 would revert on the direct call since signer would be 0x0, so metaDelegateCall_EIP1271() is implicitly protected as well