Handling of both ether and token transfers produce fragile code

[joshuahannan]

Functions like serveTx, and getRequired have the ability to deal both with ether and token transactions. To do so, some variables in the function signatures or bodies often represent more than one concept simultaneously. Such a role is determined by the implicit context of the operation in progress. Although this is an interesting feature of the wallet since it is inherent in the semantics of ethereum itself, the implementation makes the code difficult to understand on occasion and prone to overlooked unintended behaviors.
To illustrate, consider the function serveTx’s _to parameter, which is documented as @param _to Address of target. If _to is zero, the ether involved in the transaction is sent to a newly deployed contract. If _to is not zero, it may represent a destination address that will receive ether, or a token contract that will execute a certain function. More generally, it can also represent any contract that will execute an arbitrary function.
Additionally, the function getRequired shows that _to can be used as a key in the currentSpend mapping of a Transaction struct. In this case, it is used to indicate that the value to be accounted for is ether when _to is equal to zero, or a token when _to is different from zero, despite of it being documented as@param _to Target address of transaction. Looking at it form the opposite side, if an owner wanted to extract tokens from the wallet, a call to serveTx would have to be made with _to populated with the address of the token’s contract and the actual “target” receiving the tokens would have to be encoded as a parameter inside _txData.
Trying to address the fact that a transaction in ethereum can be practically anything in the wallet is certainly a difficult thing to achieve and, as we have seen, can lead to complexity, increased surface of attack and critical attack vectors in the code.
Consider correcting the documentation and naming misalignments mentioned above, and where possible, making it very clear when the code is acting in the context of a token or in the context of ether. This will reduce the attack surface of the library as well as reduce the likelihood of errors being introduced in future code updates.

[joshuahannan]

I don't think this is something we can really fix functionally but I am updating the documentation to better indicate this difference