Merge audit branches: zellic-audit => veridise-audit#188
Open
Merge audit branches: zellic-audit => veridise-audit#188
Conversation
… from a private slot (#110)
Tests are currently failing since right now we only restrict returning shielded addresses and integers.
…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.
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.
Add failing tests that document the incomplete literal assignment warning for shielded types identified in the Veridise audit report. New test files demonstrating missing warnings: - warn_shielded_literal_struct.sol: Named struct field initialization - warn_shielded_literal_struct_positional.sol: Positional struct init - warn_shielded_literal_multiple_args.sol: Multiple arguments in struct - warn_shielded_fixedbytes_literal.sol: Direct sbytes literal conversion - warn_shielded_fixedbytes_literal_struct.sol: sbytes in struct These tests initially fail, documenting the gaps: 1. Only first argument checked (not all arguments) 2. Struct constructors not checked at all 3. ShieldedFixedBytes type not checked Updated existing sbytes tests to expect new warnings: - sbytes_explicit_conversions.sol - sbytes_implicit_conversions_fail.sol - sbytes_operations.sol - sbytes_size_conversions.sol - sbytes_struct.sol After the fix, all these tests will pass with proper warnings emitted.
Fix the incomplete literal assignment warning for shielded types as identified in the Veridise audit report. The original implementation only checked the first argument and missed: - Literals in non-first positions - Literals inside struct constructors - ShieldedFixedBytes type entirely Changes: - Refactored warning logic into two clear functions: * checkLiteralToShielded() - Core warning check (replaces inline code) * checkShieldedLiteralWarning() - Recursive dispatcher for complex cases - Extended coverage to all 4 shielded types: * ShieldedBool ✓ * ShieldedAddress ✓ * ShieldedInteger ✓ * ShieldedFixedBytes ✓ (NEW) - Now checks ALL arguments, not just the first - Recursively handles struct constructors (named and positional) - Cleaner code structure for auditor review All 3,709 syntax tests pass with no regressions.
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.
Add tests to verify that copying shielded arrays uses cstore/cload instead of sstore/sload. These tests currently fail because the implementation hardcodes sstore/sload regardless of type shielding. Tests cover: - Memory-to-storage copy - Storage-to-storage copy - Calldata-to-storage copy - Multiple items per slot (sbool[]) Each test verifies: - Initial array length is 0 before copy - Array length after copy is correct - Array elements are correctly stored and retrieved Addresses: Veridise audit finding VER-771
This fixes a critical issue where storage array copy operations hardcoded sstore/sload regardless of whether the base type was shielded. For shielded value types like suint[] and sbool[], this would violate confidentiality guarantees and could cause reverts when accessing confidential storage slots. Changes: - Modified copyValueArrayToStorageFunction to select opcodes based on base type's isShielded() status - Fixed hardcoded sload in updateSrcPtr template to use parameterized loadOpcode for both single and multi-item-per-slot cases The generated IR now correctly emits cstore/cload for shielded arrays and sstore/sload for non-shielded arrays. Addresses: Veridise audit finding VER-771
We updated the semantics of confidential storage opcodes in SeismicSystems/seismic-revm#180. The changes here are needed to reflect the new semantics.
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)
Incorporates all commits from the Veridise security audit, including: - KnownStorage optimizer bug fix (VER-762) - Comprehensive literal-to-shielded warning detection - Shielded literal detection in array literal initializers - Rational shift/exp returning shielded types - Unchecked arithmetic cleanup for ShieldedInteger - Shielded type comparisons returning sbool - Arithmetic info leak warnings - Control flow condition warnings for shielded types - Implicit string literal to sbytesN rejection - ABI encoding/decoding rejection for shielded types - saddress member restrictions - Via-IR pipeline disablement - Dynamic shielded array length type changes - Plus all overlapping fixes shared with zellic-audit
Incorporates all commits from the Zellic security audit, including: - TIMESTAMPMS / block.timestamp_ms / block.timestamp_seconds support - SMT solver timestamp aliasing fix - ShieldedFixedBytes via-IR support - String literal to ShieldedFixedBytes conversion codegen - UDT wrapping shielded primitives codegen - Struct delete with mixed shielded fields fix - Bytes/string push bug fix - Address/saddress to enum conversion guard fix - CSE across confidential domain fix - CI caching improvements - Plus all overlapping fixes shared with veridise-audit # Conflicts: # .github/workflows/test.yml # libsolidity/codegen/YulUtilFunctions.cpp
… Types.h Both audit branches independently added these method declarations to UserDefinedValueType but in slightly different positions. The auto-merge kept both copies, causing a compile error.
Addresses audit recommendations to document shielded type limitations including msg.value visibility, array length metadata leakage, ABI encoding restrictions, sbool branching info leak, saddress member limitations, literal deployment leakage, and shielded dynamic bytes.
Add syntax test expecting a warning when msg.value is assigned to a shielded type, since msg.value is always publicly visible on-chain.
Emit warning 9664 when msg.value is used in an expression being assigned to a shielded type variable. msg.value is always publicly visible on-chain and wrapping it in a shielded type does not hide the transaction value from observers. Addresses audit item #2 from "Maintainability and documentation warnings for shielded behavior".
Add new syntax test and update existing tests to expect warning 9665 when a state variable is a dynamic array with shielded element types, informing users that an upper bound on the array length may be observable through gas cost analysis.
Emit warning 9665 when a state variable is a dynamic array with shielded element types. Although the length is stored in confidential storage, an upper bound may still be observable through gas cost analysis when elements are added or removed. Addresses audit finding "Array lengths are public" updated response.
Add comments to isByteArray/isByteArrayOrString noting that these helpers assume non-shielded base types. If shielded dynamic bytes are added in the future, the stride calculations and storage layout code that depends on these helpers must be revisited. Addresses audit item #5 from "Maintainability and documentation warnings for shielded behavior".
Add syntax test expecting DeclarationError 9826 when shielded types are used with transient storage. Remove old cmdline and semantic tests that expected transient shielded variables to work.
Transient storage is a separate storage space that requires special privacy considerations for traces. Reject shielded transient variables at the declaration level with DeclarationError 9826. The previous codegen fix (TSTORE/TLOAD for transient shielded) remains as defense-in-depth but is now unreachable.
cdrappi
force-pushed
the
merge-audits
branch
from
14fa488 to
b03ddb5
Compare
…ion contexts Update test expectations so warnings 9660/9661/9662/9663/1457 fire for shielded literal conversions in assignments, function arguments, return statements, and push calls — not only variable declarations. Warning source locations also narrow from the full statement span to the conversion expression span.
Move the literal-to-shielded check from endVisit(VariableDeclarationStatement) into typeCheckTypeConversionAndRetrieveReturnType, which runs for every explicit type conversion regardless of syntactic context. This ensures warnings fire for assignments, function arguments, return statements, and push calls — not only variable declarations. Remove the now-dead checkShieldedLiteralWarning helper.
Update test expectations to require warning 9660 for constant expressions (BinaryOperation, UnaryOperation) cast to shielded types, not just direct Literal AST nodes. Add comprehensive test file covering arithmetic expressions, unary negation, and sbytes32 casts.
checkLiteralToShielded gated all checks behind a Literal AST node downcast, so constant expressions like 0 ** 1E1233 (BinaryOperation) or -(2 + 3) (UnaryOperation) with RationalNumber type silently skipped the warning. Restructure the function to check RationalNumber, Enum, and ShieldedFixedBytes target types first (no Literal cast needed), then fall through to the Literal cast only for ShieldedBool and ShieldedAddress which genuinely need literal->value() and literal->looksLikeAddress().
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
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.