Parity-ethereum: Unsafe code in wallet?

[dukei]

Recent events brought my attention to the wallet source code. There are a couple of potential problems here, can you clarify?

1 . You copy calldata to memory starting from 0x0: https://github.com/paritytech/parity/blob/02d462e2636f1898df3e7556364260c594b112e6/js/src/contracts/snippets/enhanced-wallet.sol#L415

However according to http://solidity.readthedocs.io/en/develop/miscellaneous.html#layout-in-memory Solidity reserves first 96 bytes of memory. It allows using of first 64 bytes in assembly but in this case calldata is obviously more than that. So it will overwrite the pointer to free memory.

Moreover,

There are some operations in Solidity that need a temporary memory area larger than 64 bytes and therefore will not fit into the scratch space. They will be placed where the free memory points to, but given their short lifecycle, the pointer is not updated.

This means that corrupted free memory pointer may lead to memory corruption. Even if it does not happen now it might happen in one of upcoming Solidity versions.

2 . delegatecall uses the storage of the calling contract. However, Wallet does not declare all of the storage variables that are used in WalletLibrary. For example, https://github.com/paritytech/parity/blob/02d462e2636f1898df3e7556364260c594b112e6/js/src/contracts/snippets/enhanced-wallet.sol#L391 . I am not sure how the undeclared storage is allocated (if it is dynamic, it may be ok). But since it is not documented anywhere it is potentially unsafe and might lead to storage overwriting/corruption.

Can you please comment on these issues?

[gavofyork]

1. the function ends immediately following this block of assembly; the solidity compiler does no memory allocation during the unwinding of the constructor function and so there is no need to guarantee solidity's free-pointer state.

2. the solidity compiler allocates fields according to the order they are in in the source; omitting later fields doesn't cause the layout of earlier ones to change.

[dukei]

Of course it works now. I mean that these assumptions should be clarified in comments because adding or removing some code in the future may introduce more vulnerabilities.

And are you sure solidity will never do memory allocation during the unwinding?

[5chdn]

Questions should be addressed here: https://github.com/paritytech/contracts/pull/74