Open
Conversation
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.
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
cdrappi
force-pushed
the
veridise-audit
branch
from
b99b975 to
8f80457
Compare
We updated the semantics of confidential storage opcodes in SeismicSystems/seismic-revm#180. The changes here are needed to reflect the new semantics.
Add tests for delete operations on shielded types: - Basic shielded types (suint256, sint256, sbool, saddress, sbytes32) - Shielded arrays (suint256[3], saddress[4]) The array deletion test demonstrates a bug where ArrayUtils::clearArray uses hardcoded SSTORE instead of CSTORE for small shielded arrays.
Fix two code paths that were incorrectly using SSTORE for shielded types: 1. GenericStorageItem::setToZero (LValue.cpp) - Now checks all shielded types (not just suint256) - Properly uses CSTORE for saddress, sbool, sbytes32, sint256, suint256 2. ArrayUtils::clearArray (ArrayUtils.cpp) - Fixed unrolled loop optimization path for small arrays - Was hardcoded to SSTORE, now checks if base type is shielded - Affects operations like `delete suint256[3]` or `delete saddress[4]` Both fixes prevent leaking private data to public storage during delete operations.
…#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
#112) Fixes SeismicSystems/internal#177. Now that #136 had been merged, simple intra-block cstore/store semantics are prevented. However more complex cstore/sstore (such as in different assembly blocks) are not caught by the type checker. So we still want to make sure the optimizer doesn't remove SLOADs that could possibly revert at runtime. This PR achives (at least I think...) that in both: 1. the evmasm optimiser (when solc run without --via-ir) 2. libyul optimiser (when run with --via-ir) --------- Co-authored-by: Samuel Laferriere <9342524+samlaf@users.noreply.github.com>
Fixes https://app.audithub.dev/app/organizations/224/projects/585/project-viewer?issueId=767 Made SLOAD be movableExceptForSideEffects so that the yul optimiser won't hoist it out of for loops (since it would be wrong in cases where the loop might run 0 times, but hoisting it could cause a revert). Companion to #148. ## Gas costs effect This is actually potentially very bad for certain contracts... see some of the tests that get changed with a lot of function calls not getting hoisted anymore. That being said.. the tests do seem a bit artificial.
Fixes https://app.audithub.dev/app/organizations/224/projects/585/project-viewer?issueId=763 and https://app.audithub.dev/app/organizations/224/projects/585/project-viewer?issueId=772 Companion to #151 SLOADs can revert in seismic-solidity when trying to read a confidential slot. We thus mark it as having effects so that SLOADs that could revert at runtime are not removed by optimizers (peephole, cse, etc). Added a large comment in Instruction.cpp to explain the tradeoff here, and the fact that we might want to take a more fine-grained approach if we find the gas costs unnaceptable. ## Why this is important Here's an example brought up by Benjamin Sepanski from veridise: > For example, the UnusedPruner could remove a trailing SLOAD which is intended to be used by a user to force a revert in case the data is confidential ## Tests Had to "fix" a bunch of the tests. Made separate commits for the different types of tests, so worth looking at individual commits for the different things that need to change because of this coarse grained approach (making SLOAD have side effects instead of just fixing the optimisers directly to keep certain optimizations possible). I use "fix" in quotes because we are basically getting rid of many optimizations because of making SLOAD have side effects. We might want to benchmark against some set of contracts to see whether this is acceptable or not before pulling the trigger. Otherwise we might need to implement a more fine-grained solution. I think the main issue this might cause is that the optimizer can no longer transform "SLOAD(x) SLOAD(x)" into "SLOAD(x) DUP", so we incur an extra SLOAD when loading from the same slot. Although maybe not that bad since second is a warm access which is only 100 gas..?
Add syntax tests to verify warnings are emitted for shielded literals in array literal initializers: - warn_shielded_literal_array.sol: basic array literals - warn_shielded_literal_array_nested.sol: nested array literals
Extend checkShieldedLiteralWarning() to handle TupleExpression (array
literals). The previous fix only handled FunctionCall expressions,
missing cases like:
suint[2] memory a = [suint(1), suint(2)];
This now correctly emits warnings for shielded literals inside array
literal initializations, including nested arrays.
Addresses remaining issue in Veridise audit "Literal assignment to
shielded warning is incomplete".
Add tests that expose bugs in VER-771 (Yul array utils use SSTORE). The original fix addressed copyValueArrayToStorageFunction but missed several other locations: 1. bytes_push_with_value.sol - Tests bytes.push(value) including the long array case (>31 elements) which exposes the bug where sload was used instead of sstore in storageArrayPushFunction. 2. shielded_array_push_with_value.sol - Tests that pushing values to shielded arrays uses cstore for the actual storage write. 3. shielded_compact_array_abi_encode.sol - Tests that ABI encoding compact shielded storage arrays (like sbool[]) uses cload. 4. shielded_struct_abi_encode.sol - Tests that ABI encoding structs with shielded members uses cload for the shielded fields. These tests will fail without the corresponding fix.
This completes the fix for VER-771 (Yul array utils use SSTORE) which
was partially addressed in a previous commit. The original fix only
covered copyValueArrayToStorageFunction, but several other locations
were missed.
Changes:
1. YulUtilFunctions.cpp - storageArrayPushFunction():
- Fixed sload(array, add(data, 2)) -> sstore() in byte array path
(sload only takes 1 argument, this was broken code)
- Fixed storeValue -> <storeValue> to use templated function name
2. ABIFunctions.cpp - abiEncodingFunctionCompactStorageArray():
- Changed hardcoded sload to <loadOpcode> template variable
- Added loadOpcode selection based on _from.baseType()->isShielded()
3. ABIFunctions.cpp - abiEncodingFunctionStruct():
- Changed hardcoded sload to select cload/sload based on
memberTypeFrom->isShielded()
These changes ensure that:
- Shielded storage arrays use cload/cstore for confidential data
- Non-shielded arrays continue to use sload/sstore
- The bytes.push(value) functionality works correctly for long arrays
Add regression tests verifying that transient shielded types use TSTORE/TLOAD instead of CSTORE/CLOAD. Command-line tests: - evmasm_transient_storage_shielded_suint256 - evmasm_transient_storage_shielded_sint256 - evmasm_transient_storage_delete_shielded Semantic tests: - transient_shielded_state_variable.sol - transient_shielded_different_types.sol
Emit warning (5765) when shielded types (sbool) are used in branching
conditions that can leak information through observable execution
patterns such as gas costs, state changes, and execution traces.
This addresses the audit finding about implicit behavior for shielded
types that may surprise users and make privacy boundaries unclear.
Affected constructs:
- if/else conditions: if (sbool_expr) { ... }
- while loops: while (sbool_expr) { ... }
- for loops: for (; sbool_expr; ) { ... }
- ternary expressions: sbool_expr ? a : b
- require()/assert() calls: require(sbool_expr)
Add semantic and syntax tests for explicit conversion from string
and hex literals to shielded fixed bytes types (sbytesN).
These tests expose the internal compiler error that occurs when
converting literals like sbytes16("hello") - the compiler currently
lacks handling for StringLiteral -> ShieldedFixedBytes conversions
in both legacy and via-ir pipelines.
Handle StringLiteral -> ShieldedFixedBytes in both legacy and via-ir
codegen pipelines by extending the existing FixedBytes handling.
In CompilerUtils::convertType (legacy) and
YulUtilFunctions::conversionFunctionSpecial (via-ir), the condition
for handling string literal to fixed bytes conversion now also checks
for ShieldedFixedBytes. Since ShieldedFixedBytesType inherits from
FixedBytesType, the existing numBytes() call and conversion logic
work correctly for both types.
This fixes the internal compiler error when using expressions like:
sbytes16 x = sbytes16("hello");
sbytes32 y = sbytes32(hex"abcd");
Add a syntax test that verifies string literals cannot be implicitly converted to shielded fixed bytes types (sbytesN). The test confirms that sbytes1 a = "a" and similar assignments are rejected, while regular bytes1 d = "a" implicit conversions continue to work.
Block implicit conversion from string literals to ShieldedFixedBytesType
in StringLiteralType::isImplicitlyConvertibleTo. Since ShieldedFixedBytesType
inherits FixedBytesType, the existing dynamic_cast<FixedBytesType*> check
was matching shielded types, allowing sbytes4 x = "ABCD" without an
explicit cast.
Add a ShieldedFixedBytesType guard that returns false before the
FixedBytesType size check. Also add isExplicitlyConvertibleTo override
to StringLiteralType so explicit casts like sbytes4("ABCD") still work.
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)
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.
…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.