magic values, clone function name

[bennetyee]

kmaas/contracts/AccountWithSymKey.sol

Line 16 in a4b924c

mstore(contractClone, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)

having magic values makes code less readable. auditors will know to put it through a solidity decompiler but for readability / transparency it wouldn't hurt to have comments explaining that this is the EVM byte code for the contract creation program -- to the F3 byte -- and thereafter is the contract code, with the target contract address interpolated, for the so-called "cloned" contract code.

the clone function is also a bit of a misnomer, since it doesn't clone a process like linux clone syscall or create a duplicate of anything; instead, the contract code is a short forwarding contract that uses DELEGATECALL to forward all calls to the target, basically providing a private address space for the code in the target contract to enable having a separate instance.

is the reason to provide the clone code to make deploying instances cheaper -- instead of deploying a copy of the target contract, it's just a call to clone factory method? but every call requires copying the call args from msg.data to memory to forward via the DELEGATECALL and dealing with copy the returndata to memory on return to use with the RETURN instruction, so using this adds to every future call to the contract methods. if the cloned contract is used frequently this appears to trade off one-time contract creation cost vs per-call costs, i.e., lower constant term for a higher linear term, which seems like a bad choice, even if the reduced readability of having the magic values is ignored. comments about the design trade-offs justifying this decision would be nice.

in general, comments -- that are accurate and updated when code is updated -- aid in readability and transparency. auditors might not trust comments and ignore them, or verify their correctness, but that's not much extra work. ideally, a source forwarding contract -- even if it's just EVM assembly -- that compiles to the magic byte sequence here would be great documentation, even if there's no automation to use that as source code to generate the solidity code with the values (i.e., this file would be the output of that compilation step).

this applies to

kmaas/contracts/Account.sol

Line 14 in a4b924c

function clone(address target)

as well, since that's another copypasta of clone