Ensure CodeDeployer pseudo-contracts are not executable

[EndymionJkb]

Description

We received an interesting report from Czar102 about the CodeDeployer library. Since it only happens on factory deployment, it's not a vulnerability, and the likelihood of this actually happening is low. Nevertheless, it is a valid issue with a non-zero probability of happening, so we will fix it.

CodeDeployer is a library which "deploys" arbitrary code to an address; it need not be a valid contract. We use this in the BaseSplitCodeFactory, which underlies all our pool factories, so that we are able to deploy contracts with very large creationCode: up to 48k, twice the contract size limit. This is needed for some contracts with very tight bytecode constraints, and also a large amount of initialization code (e.g., filling in immutables).

Essentially, it splits the bytecode in half, uses CodeDeployer to deploy each "contract half" separately, then rejoins them for the actual deployment. Every time a new pool is deployed, it reassembles the bytecode from these two contracts, then appends the constructor arguments for the specific pool.

Since the BaseSplitCodeFactory logic splits the bytecode exactly in half, the second half could have any random sequence of bytes. Depending on the layout (which could theoretically change in the future), it might be in the middle of a string or data section.

So here's the problem: what if that random sequence of bytes accidentally (or maliciously) happens to start with valid opcodes? Sending a transaction would try to execute it, and it wouldn't necessarily revert. If it happened to contain a "selfdestruct" opcode, it would destroy itself, effectively deleting the second half of the pool's bytecode, and bricking the factory.

To ensure that the second "half" can never be executed, "protect" it by prepending an invalid opcode. Rather than have the option, it could just always do it, but there might be use cases where exact fidelity is important.

In BaseSplitCodeFactory, we apply the protection to the second "half" of the bytecode, and then skip that extra byte during reassembly.

Type of change

• Bug fix
• New feature
• Breaking change
• Dependency changes
• Code refactor / cleanup
• Documentation or wording changes
• Other

Checklist:

• The diff is legible and has no extraneous changes
• Complex code has been commented, including external interfaces
• Tests are included for all code paths
• The base branch is either master, or there's a description of how to merge

Issue Resolution

[nventuro]

Huh, I hadn't thought of this. Very interesting, nice find @Czar102.

[Czar102]

Hi, thanks for tagging me.
Looking into the code, I can see two issues emerging here.

1. The first half of the code can also selfdestruct (for example because the code is prepended with a selfdestruct conditional to the length of the code specific to the deployed one). I don't believe this would ever happen to a contract by accident, nevertheless if the library could be fully relied upon, there should be no way to hide a surprise when using it. I would recommend using execution prevention on the first half of the code, too.
2. I stumbled upon this bug when reviewing smart contracts for another project. If they are other users, they may potentially use it in upgradeable contracts, with a modified constructor. In this case, if they upgrade the library, the second half of the code will be shifted by one byte (because the new implementations skims the code by one byte) (first byte will be the second byte of the code and the last byte of the code deployed will be 0x00), corrupting the code. This situation is extremely unlikely, but I'm not sure how popular this library is in DeFi. If you wanted to be fully compatible with past versions in that sense, consider checking the extcodesize of both parts (or just one) and checking it/them against immutably stored sizes.

Apart from that, the fix looks good to me.

[EndymionJkb]

Hi, thanks for tagging me.
We wanted to reward the creativity here somehow :)

1. The first half of the code can also selfdestruct (for example because the code is prepended with a selfdestruct conditional to the length of the code specific to the deployed one). I don't believe this would ever happen to a contract by accident, nevertheless if the library could be fully relied upon, there should be no way to hide a surprise when using it. I would recommend using execution prevention on the first half of the code, too.

I thought of that (see comments in the code), and it seemed like overkill in this case. We know our contracts are never going to have a selfdestruct in the actual code, and the first half is the beginning of the real contract. I suppose if a project did want to deploy contracts with selfdestruct opcodes, it could be a concern. It wouldn't be hard to do.

In any case we do check both halves in the fork tests for all pools now, so if the first half did somehow blow up, we would catch it.

[jubeira]

LGTM, thanks @EndymionJkb and @Czar102 !

Personally I'm fine with the fix. This contract was created with pool factories in mind, and it should be more than fine for that use case.