The emitted constant name is unvalidated while the contract name is, in the same generated file

[thedavidmeister]

Audit scope: whole-repo, commit 7aa85a4

Dimension 6 (hazard surface) · severity low

Where

• src/lib/LibCodeGen.sol:275-293,303-320,331-349,360-378
• test/lib/LibCodeGen.bytesConstantString.t.sol:89-97 (test/src/lib/ after Move every .t.sol into the test/src/lib mirror tree #56)

Problem

An identifier the library interpolates verbatim into generated Solidity is
checked on one path and unchecked on another, so a bad name reaches the file as
source text rather than as a revert.

requireContractName guards the contract name on every path that uses it
(pathForContract, describedByMetaHashConstantString), and its NatSpec is
explicit that being a Solidity identifier is what makes interpolation safe. The
name parameter of bytesConstantString, uint8ConstantString,
bytes32ConstantString and addressConstantString is interpolated into the same
generated file with no check at all. The tests confirm this is the current
contract rather than an oversight: testBytesConstantStringMatchesMeasuredLine
fuzzes name as an arbitrary string and asserts only that it is emitted
verbatim.

Scenario: a consumer derives constant names from data — a word list, a meta file,
a tag string (st0x.deploy's tagSuffix() already builds names from a tag). A
name carrying a space, a ;, or a - produces a file that either fails to
compile with an error pointing at generated code nobody wrote, or — with a ; —
compiles into a different set of declarations than intended.

Proposed fix

function bytesConstantString(Vm vm, string memory comment, string memory name, bytes memory data)
    internal
    pure
    returns (string memory)
{
    requireContractName(name);
    ...
}

in all four emitters. requireContractName already implements exactly the
Solidity-identifier rule a constant name must satisfy; if the shared name reads
badly, rename it requireIdentifier and keep one implementation.