Merge audit branches: zellic-audit => veridise-audit#200
Open
cdrappi wants to merge 75 commits intoveridise-auditfrom
Open
Merge audit branches: zellic-audit => veridise-audit#200cdrappi wants to merge 75 commits intoveridise-auditfrom
cdrappi wants to merge 75 commits intoveridise-auditfrom
Conversation
… from a private slot (#110)
This test demonstrates the bug where ArrayUtils::clearArray uses hardcoded SSTORE instead of CSTORE for small shielded arrays. The test shows that deleting shielded arrays like suint256[3] or saddress[4] incorrectly generates SSTORE instructions in the unrolled loop optimization path. This will be fixed in the next commit.
The original fix in commit 5c75476 addressed GenericStorageItem::setToZero, but ArrayUtils::clearArray had an optimization path that bypassed it. For small fixed-size arrays (storageSize <= 5), clearArray uses an unrolled loop that directly emits store instructions. This code path was using hardcoded SSTORE without checking if the base type is shielded. This meant that operations like `delete suint256[3]` would incorrectly use SSTORE instead of CSTORE, leaking private data to public storage. The fix checks if the array's base type is shielded and uses CSTORE for shielded types, SSTORE for non-shielded types. Affected code path: - ArrayUtils::clearArray lines 568-578 (unrolled loop for small arrays) Other code paths correctly use StorageItem::setToZero which handles shielded types properly after the original fix.
Tests are currently failing since right now we only restrict returning shielded addresses and integers.
4 of the tests were relying on returning shielded types, which now need to be modified to unshield the types before returning them from public functions.
…al fcts This fixes veridise issue 759. Moved the check from DeclarationTypeChecker to TypeChecker because with the new semantic we need to recursively check all child AST nodes (members of structs for example). Keeping the check in DeclarationTypeChecker was throwing some assert errors on some children nodes that haven't been previously been type checked themselves. Having the check done in TypeChecker also feels more natural anyways, since its checking for return types and there was already a function named checkArgumentAndReturnParameter.
Verifies that the timestampms instruction is rejected when targeting EVM versions older than Mercury.
Add TIMESTAMPMS to the same version gate as CLOAD/CSTORE in both EVMVersion::hasOpcode() and AsmAnalysis. This prevents generation of bytecode containing opcode 0x4b for VMs that don't support it.
…y typechecker level
Add EVM version checks in TypeChecker.cpp for timestamp_ms and timestamp_seconds magic block members. Users targeting non-Mercury VMs will now get a clear compile-time error instead of generating bytecode with invalid opcodes that crash at runtime.
The SMT model creates separate symbolic variables for block.timestamp and block.timestamp_seconds even though they resolve to the same opcode. This causes `assert(block.timestamp == block.timestamp_seconds)` to fail spuriously in the SMT checker.
The SMT encoder was creating separate symbolic variables for block.timestamp and block.timestamp_seconds, even though they resolve to the same TIMESTAMP opcode at runtime. This could cause spurious assertion failures when both were used in the same contract. Following the existing pattern for difficulty/prevrandao aliasing: - SMTEncoder.cpp: alias timestamp_seconds → timestamp in both Identifier and MagicType code paths - Predicate.cpp: alias in TxVarsVisitor for counterexample formatting - SymbolicState.cpp: remove independent constraint for timestamp_seconds - SymbolicTypes.cpp: remove timestamp_seconds from transaction member types
…#136) Add some basic shielded storage type checking to solc's analysis phase so as to prevent compilation of programs that would clearly halt at runtime (for example trying to sload a private slot). This is effectively adding (very basic) type checking for the new opcode semantic rules that were recently changed in revm: SeismicSystems/seismic-revm#180 ## Tests Moved a few semantic tests to become syntaxTests, since the same programs now fail to compile (with a type error) and so can't be input to revm in semantic tests. ## Notes This PR will affect #112 since the semantic tests there will no longer be able to be used since they won't type check... 2 solutions: 1. we merge that PR without tests 2. we try to come up with some test that would trick the basic type-checker implemented by this PR yet still generate a CSTORE followed by SSTORE that would then need to fail at runtime in revm. Question at that point though is whether that semantic test shouldnt become a syntax test and the type-checker here shouldnt be augmented to catch that... :D
The guard in CompilerUtils::convertType used || instead of &&, making the assertion always true (a tautology). The condition `(A != X || A != Y)` is always true because a value cannot equal both X and Y simultaneously. Changed to && so the assertion correctly fails when the type is either Address or ShieldedAddress.
Add additional semantic tests for the storageArrayPushFunction bug where sload was incorrectly used instead of sstore and storeValue was missing template brackets for long byte/string arrays. Tests cover: - bytes_push_long_array: pushing to arrays already in long encoding - bytes_push_boundary_transition: short-to-long array transition - bytes_push_length_update: verifies length updates correctly - string_push_via_bytes: tests string path via bytes(str).push()
…ed types Verify that bytes/string storage operations (push, pop, resize, assign) compile correctly. These operations use byte array helpers that should only emit sload/sstore since there is no shielded dynamic bytes type (sbytesN is a fixed-size value type, not a dynamic byte array).
…opcode selection Four byte array helper functions (resizeDynamicByteArrayFunction, increaseByteArraySizeFunction, byteArrayTransitLongToShortFunction, storageByteArrayPopFunction) chose between sload/cload and sstore/cstore based on _type.category() == ShieldedInteger. Since _type is an ArrayType, its category is always Array, making the shielded branch unreachable. Fix by checking _type.baseType()->isShielded() instead, which correctly inspects the element type. This has no behavioral change today (bytes/string have a non-shielded base type) but prepares these helpers for a future shielded dynamic bytes type.
Add tests verifying the compiler rejects shielded types in: - abi.encode() with suint256, saddress, sbool, structs, and arrays - abi.encodePacked() with shielded types - abi.encodeWithSelector() and abi.encodeWithSignature() - abi.decode() with shielded type targets Update existing tests to expect the new error and cast shielded NONCE to uint96 in AESEncryptAndDecryptWithRng semantic test. These tests expose a vulnerability where shielded values could be serialized to plaintext bytes via ABI encoding, leaking confidential data and allowing recovery via abi.decode.
Add type checks in TypeChecker to prevent shielded values from being serialized through ABI encoding functions, which would leak confidential data as plaintext bytes. Changes: - typeCheckABIDecodeAndRetrieveReturnType(): reject shielded types in abi.decode() target types (error 4851) - typeCheckABIEncodeFunctions(): reject shielded types in abi.encode(), abi.encodePacked(), abi.encodeWithSelector(), and abi.encodeWithSignature() (error 3648) - typeCheckABIEncodeCallFunction(): reject shielded types in abi.encodeCall() arguments (error 3648) Uses containsShieldedType() to catch both direct shielded types and composite types (structs, arrays) containing shielded fields. Checks are guarded with else-if to only trigger when the type would otherwise be valid for encoding, avoiding interference with existing error paths.
…ented) shielded bytes (#176) Replace remaining hardcoded sstore/sload with templated storeOpcode/ loadOpcode in byte array storage helpers, using baseType()->isShielded() to select between cstore/cload and sstore/sload. This sets up dead code to be correct for when sbytes (dynamic shielded bytes) is implemented. Functions fixed: - copyByteArrayToStorageFunction: 5 sstore + 1 sload + read template - decreaseByteArraySizeFunction: 2 sstore - increaseByteArraySizeFunction: 3 sstore - storageByteArrayPopFunction: 2 sstore - storageArrayPushFunction: 5 sstore/sload in byte array path
…lded fields Covers the case where delete on a struct containing both shielded and non-shielded fields incorrectly clears packed non-shielded slots with cstore instead of sstore.
clearStorageStructFunction used containsShieldedType() as a struct-level flag to choose cstore vs sstore for all slot clears. For mixed structs this caused non-shielded packed fields to be cleared with cstore while their getters use sload, reverting under privacy rules. Since all shielded types have storageBytes() == 32, the storageBytes < 32 branch can only ever process non-shielded fields. Hardcode sstore in that path and remove the unused struct-level flag.
Add 4 tests covering the auditor's finding that mixing cstore/sload in inline assembly compiles but reverts at runtime: - shielded_cstore_caller_then_sload: auditor's exact POC (TypeError 5768) - shielded_cstore_then_sload_var_slot: variable-based slot matching (TypeError 5768) - shielded_cstore_then_sload_dynamic_slot_ok: documents known limitation where dynamic slots cannot be caught at compile time - cstore_then_sload_dynamic_slot_should_fail: proves dynamic slot case correctly reverts at runtime The core fix already exists in validateShieldedStorageOps() (ad883e1). These tests confirm coverage of the auditor's specific scenarios and the acknowledged limitation around dynamic slot expressions.
Update all tests involving dynamic shielded arrays to expect .length to return suint256 instead of uint256, and to use cload/cstore instead of sload/sstore for the length slot in inline assembly. Add new semantic and syntax tests for comprehensive coverage of the suint length behavior including type checking, cload/cstore assembly verification, push/pop operations, and mapping interactions.
Dynamic shielded arrays (e.g. suint[], saddress[]) now store their length in confidential storage (cload/cstore) instead of public storage (sload/sstore), and .length returns suint256 instead of uint256. This prevents leaking collection size metadata through storage access patterns. Fixed-length shielded arrays (e.g. suint[5]) are unaffected — their .length remains uint256 (compile-time constant). All conditionals use containsShieldedType() rather than checking the base type directly, so future sbytes support will automatically inherit shielded length behavior. Changes: - Types.cpp: .length member returns suint256 for dynamic shielded arrays - ArrayUtils.cpp: 6 locations switch sload/sstore to cload/cstore for length in legacy codegen (copyArrayToStorage, clearDynamicArray, resizeDynamicArray, incrementDynamicArraySize, popStorageArrayElement, retrieveLength) - YulUtilFunctions.cpp: 5 locations switch sload/sstore to cload/cstore for length in Yul codegen (arrayLengthFunction, resizeArrayFunction, storageArrayPopFunction, storageArrayPushFunction, storageArrayPushZeroFunction)
…ion contexts Update test expectations to include warnings (9660, 9661, 9662, 9663, 1457) when shielded type conversions from literals or constant expressions appear in assignments, function arguments, return statements, and other non-declaration contexts. Add new test files for struct, array, nested array, fixedbytes, and constant expression literal-to-shielded warnings.
Move literal-to-shielded warning check from endVisit(VariableDeclarationStatement) into typeCheckTypeConversionAndRetrieveReturnType so it fires for all explicit type conversions — assignments, function arguments, return statements, and array pushes — not just variable declarations. Extract the warning logic into a reusable checkLiteralToShielded helper that checks annotation().type-based cases (RationalNumber, Enum) before the Literal AST node cast, allowing constant expressions like 0 ** 1E1233 to also trigger warnings. Add checkMsgValueToShielded helper for msg.value-to-shielded warnings.
Run semantic tests in 4 configurations: 1. No optimizer, no IR (baseline) 2. With optimizer, no IR (baseline) 3. No optimizer, --via-ir (tests IR codegen) 4. With optimizer, --via-ir (tests optimized IR)
- shielded_array_storage_pop_zero_length.sol: Change EVMVersion from >=petersburg to >=mercury (suint[] requires CLOAD/CSTORE opcodes) - all_possible_user_defined_value_types_with_operators.sol: Add compileViaYul:false with explanatory comment (test exceeds EIP-170's 24KB contract size limit when compiled with via-IR without optimizer)
Add 4 regression tests with compileViaYul:also to ensure via-IR specific bugs are caught while maintaining non-IR test coverage: 1. shielded_struct_delete_mixed_viair.sol Tests that struct delete uses correct opcodes (sstore for uint16/uint8, cstore for sbytes1) when clearing mixed shielded/non-shielded fields. Prevents regression of clearStorageStructFunction bug. 2. sbytes_dynamic_packed_storage_viair.sol Tests sbytes1 array operations (push, index read, index write) use numBytes() not storageBytes() for shift/mask in packed storage. Prevents regression of packed storage bugs. 3. sbytes_abi_encoding_viair.sol Tests storage-to-memory copy uses cload not sload for sbytes arrays. Prevents regression of abiEncodingFunctionCompactStorageArray bug. 4. timestamp_magic_viair.sol Tests block.timestamp_seconds and block.timestamp_ms generate correct IR (timestamp() and timestampms() respectively). Prevents regression of IR timestamp member bug. All tests use compileViaYul:also to run in both IR and non-IR modes, ensuring comprehensive coverage and regression prevention.
Fixes all remaining issues blocking via-IR compilation for shielded types. Test results: 1641/1642 pass without optimizer (99.94%), 1642/1642 with optimizer (100%). Single failure is unrelated contract size limit. Issue 1: clearStorageStructFunction used wrong opcode for mixed structs - Problem: Mixed structs (uint16 + sbytes) used cstore for ALL fields - Fix: All shielded types have storageBytes()==32, so <32 branch is non-shielded only. Hardcode sstore with defensive assertion. - File: libsolidity/codegen/YulUtilFunctions.cpp:1915-1931 Issue 2: IR timestamp magic members called non-existent functions - Problem: Generated IR called timestamp_seconds() and timestamp_ms() - Fix: Map timestamp_seconds to timestamp(), timestamp_ms to timestampms() - File: libsolidity/codegen/ir/IRGeneratorForStatements.cpp:1979-1984 Issue 3: ABI routing bug for sbytes storage arrays - Problem: sbytes has storageBytes()==32 but IS a byte array - Fix: Check isByteArrayOrString() before storageBytes() check - File: libsolidity/codegen/ABIFunctions.cpp:328-335 Issue 4: Storage-to-memory copy used sload instead of cload - Problem: abiEncodingFunctionCompactStorageArray used sload for sbytes - Fix: Add loadOpcode template parameter checking isShielded() - File: libsolidity/codegen/ABIFunctions.cpp:727 Issue 5-7: Packed storage operations used storageBytes() not numBytes() - Problem: sbytes1 has storageBytes()=32 but occupies 1 byte in arrays - Fix: Use numBytes() for shift/mask calculations in packed storage - Files: libsolidity/codegen/YulUtilFunctions.cpp:3180,3235,2978 Issue 8: Memory byte array write rejected sbytes1 - Problem: Assertion expected only bytes1 for memory byte array elements - Fix: Accept both bytes1 and sbytes1 - File: libsolidity/codegen/ir/IRGeneratorForStatements.cpp:3233 Additional: Add --unsafe-via-ir flag to CLI and Standard JSON - Allows experimental use of via-IR pipeline with shielded types - Files: solc/CommandLineParser.cpp, libsolidity/interface/StandardCompiler.cpp
cdrappi
force-pushed
the
merge-audits-final
branch
from
9f988d3 to
f952b89
Compare
Resolved 52 conflicts: - Test files: kept veridise's comprehensive privacy warnings (--ours) - .github/workflows/test.yml: updated branch targets to merge-audits-final - libsolidity/analysis/TypeChecker.cpp: removed duplicate function definitions - libsolidity/ast/Types.h: removed duplicate declarations - Removed obsolete transient storage test files replaced by veridise version All implementation code merged cleanly with no manual intervention needed.
cdrappi
force-pushed
the
merge-audits-final
branch
from
f952b89 to
d029186
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.