LibFs.buildFileForContract unlinks the existing file before the content is computed, so a failed build destroys the previously generated file

[thedavidmeister]

Audit scope: whole-repo, commit 7aa85a4

Dimension 5 (correctness/intent) · medium

src/lib/LibFs.sol:64-76

Problem

The unlink runs as its own statement (68-71) before the file content is
computed — LibCodeGen.bytecodeHashConstantString(vm, instance) is only
evaluated as an argument to vm.writeFile at 73-75. That call reverts with
CodelessInstance for a codeless instance, and cheatcode filesystem effects
are not rolled back by the EVM revert, so a build that fails destroys the
previously generated file and writes nothing back.

The consequence is a red build whose visible symptom is a missing or drifted
committed artifact rather than the actual CodelessInstance cause.

Evidence

Measured: pre-existing file at the path, buildFileForContract(vm, address(0xdead), …) reverts, vm.exists(path) == false afterwards.

Proposed fix

Verified — with it, the pre-existing file survives byte-identical:

        string memory path = pathForContract(contractName);
        string memory content =
            string.concat(LibCodeGen.filePrefix(), LibCodeGen.bytecodeHashConstantString(vm, instance), body);
        //forge-lint: disable-next-line(unsafe-cheatcode)
        vm.createDir(GENERATED_DIR, true);
        if (vm.exists(path)) {
            //forge-lint: disable-next-line(unsafe-cheatcode)
            vm.removeFile(path);
        }
        //forge-lint: disable-next-line(unsafe-cheatcode)
        vm.writeFile(path, content);

plus a test in LibFsBuildFileForContractTest:

    /// Generating the content can revert — a codeless instance has no bytecode
    /// hash to name. Nothing is unlinked until there is content to write, so a
    /// build that fails leaves the file it was about to replace intact.
    function testBuildFileForContractFailedBuildKeepsExistingFile() external {
        string memory name = "LibFsBuildFailedKeeps";
        string memory path = LibFs.pathForContract(name);
        cleanup(name);
        vm.writeFile(path, "PRE-EXISTING");

        vm.expectRevert(abi.encodeWithSelector(CodelessInstance.selector, address(uint160(0xdead))));
        iExternal.buildFileForContract(vm, address(uint160(0xdead)), name, "\n// body\n");

        assertTrue(vm.exists(path), "a failed build destroyed the existing file");
        assertEq(vm.readFile(path), "PRE-EXISTING", "a failed build rewrote the existing file");
        cleanup(name);
    }