tokencard
Recursive batched meta transactions
The executeTransactions() function can interact in unpredictable ways with the
executeRelayedTransaction() function. For example, the executeRelayed- Transaction() function can call the executeTransactions() function as address(this),
which can subsequently recurse into executeTransaction() or executeTransac- tions() indefinitely. It is unlikely that the Wallet code and all of its dependencies
are entirely reentrancy-safe; there is a substantial amount of attack surface available for an attacker with a leaked owner key to exploit. The executeTransaction() function should probably include the following:
require(_destination != address(this));
Wrt this thing, we talked about this internally to death. At the time we thought that there was no real exploit here, but to be fair it is complicated and it is hard to rule out.
It is important to note, that it is key for executeTransaction() to be able to call (by proxying a call to itself) as this is the whole point of executeTransaction(). The main reason we have built it like it is executeRelayedTransaction -> executeTransactions()-> executeTransaction() is so that we can do “Gasless Batched, Consecutive operations on our wallet or on a given dapp”, as a result executeTransaction() needs to be able to call itself address(this). For example, the user signs a executeTransactions() which has 2 executeTransaction()’s which are a) addToWhitelist b) set dailylimit e.g., this requires executeTransaction to call itself.
That said, we don’t think that executeTransaction() should be able to do another executeTransaction() or a executeTransactions(), these would be recursive calls and ultimately would do nothing but waste gas. So, it would be a matter of
require(_destination != address(this) && (signature NOT equal to (executeTransaction, executeTransactions)));
It should also be noted that we will only ever be signing relayed transactions authored by our app. Furthermore, executeTransaction calling executeTransaction is just meaningless and probably harmless due to the linear nature in which contract calls are executed at the EVM level. But executeTransaction, calling executeTransactions could have further implications ???
Anyways, can you think of any specific examples of executeTransaction() calling itself that causes any bugs/issues, or are we just being cautious (which is fine to be honest, am happy to be cautious). But you see why we need executionTransaction call itself.
