diff --git a/foundry.toml b/foundry.toml index 6d3c4ce41..80f85e121 100644 --- a/foundry.toml +++ b/foundry.toml @@ -5,10 +5,12 @@ script = 'scripts' out = 'out' libs = ['lib'] remappings = [] -fs_permissions = [{ access = "read-write", path = "./reports" }] +fs_permissions = [{ access = "read-write", path = "./reports" }, { access = "read", path = "./out" }] ffi = true evm_version = 'cancun' decode_external_storage = true +allow_internal_expect_revert = true +extra_output = ["storageLayout"] [profile.zksync] src = 'zksync/src' diff --git a/src/ProtocolV2TestBase.sol b/src/ProtocolV2TestBase.sol index bde085b7a..9e2a2ff6c 100644 --- a/src/ProtocolV2TestBase.sol +++ b/src/ProtocolV2TestBase.sol @@ -7,6 +7,7 @@ import {ReserveConfiguration} from 'aave-v3-origin/contracts/protocol/libraries/ import {IERC20} from 'openzeppelin-contracts/contracts/token/ERC20/IERC20.sol'; import {IERC20Metadata} from 'openzeppelin-contracts/contracts/token/ERC20/extensions/IERC20Metadata.sol'; import {SafeERC20} from 'openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol'; +import {Strings} from 'openzeppelin-contracts/contracts/utils/Strings.sol'; import {AaveV2EthereumAMM} from 'aave-address-book/AaveV2EthereumAMM.sol'; import {AaveV2EthereumAssets} from 'aave-address-book/AaveV2Ethereum.sol'; import {DiffUtils} from './DiffUtils.sol'; @@ -85,6 +86,10 @@ contract ProtocolV2TestBase is CommonTestBase, SeatbeltUtils, DiffUtils { string memory afterString = string(abi.encodePacked(reportName, '_after')); ReserveConfig[] memory configAfter = createConfigurationSnapshot(afterString, pool); + + // as executor does delegateCall to the payload, the payload should have no storage variable + _validateNoPayloadStorageSlots(payload); + vm.writeJson( vm.serializeString('root', 'raw', rawDiff), // output string(abi.encodePacked('./reports/', afterString, '.json')) @@ -993,6 +998,41 @@ contract ProtocolV2TestBase is CommonTestBase, SeatbeltUtils, DiffUtils { ); } + /** + * @dev Validates that a payload contract declares no state variables by inspecting the + * compiler-generated storage layout from the build artifact. + * + * If the artifact cannot be resolved (e.g. the contract was not compiled locally), + * you can skip this test manually by overriding this virtual method in your test + * + * Requires foundry.toml to have: + * extra_output = ["storageLayout"] + * fs_permissions includes { access = "read", path = "./out" } + */ + function _validateNoPayloadStorageSlots(address payload) internal view virtual { + string memory artifactPath = vm.getArtifactPathByDeployedCode(payload.code); + string memory artifact = vm.readFile(artifactPath); + + // vm.parseJson ABI-encodes the JSON value at the given key. + // A dynamic array is encoded as [uint256 offset][uint256 length][elements...], so + // decoding the first two words as (uint256, uint256) yields (offset, arrayLength). + bytes memory storageEncoded = vm.parseJson(artifact, '.storageLayout.storage'); + (, uint256 storageLength) = abi.decode(storageEncoded, (uint256, uint256)); + + require( + storageLength == 0, + string( + abi.encodePacked( + 'PAYLOAD_MUST_NOT_HAVE_STORAGE_VARIABLES: ', + artifactPath, + ' declares ', + Strings.toString(storageLength), + ' storage slot(s)' + ) + ) + ); + } + function _isInUint256Array( uint256[] memory haystack, uint256 needle diff --git a/src/ProtocolV3TestBase.sol b/src/ProtocolV3TestBase.sol index 8ef59c260..9c9166082 100644 --- a/src/ProtocolV3TestBase.sol +++ b/src/ProtocolV3TestBase.sol @@ -96,6 +96,9 @@ contract ProtocolV3TestBase is RawProtocolV3TestBase, SeatbeltUtils, CommonTestB ReserveConfig[] memory configAfter = createConfigurationSnapshot(afterString, pool); + // as executor does delegateCall to the payload, the payload should have no storage variable + _validateNoPayloadStorageSlots(payload); + { string memory rawDiff = vm.getStateDiffJson(); vm.writeJson(rawDiff, string(abi.encodePacked('./reports/', afterString, '.json')), '$.raw'); @@ -729,4 +732,39 @@ contract ProtocolV3TestBase is RawProtocolV3TestBase, SeatbeltUtils, CommonTestB vm.stopPrank(); } + + /** + * @dev Validates that a payload contract declares no state variables by inspecting the + * compiler-generated storage layout from the build artifact. + * + * If the artifact cannot be resolved (e.g. the contract was not compiled locally), + * you can skip this test manually by overriding this virtual method in your test + * + * Requires foundry.toml to have: + * extra_output = ["storageLayout"] + * fs_permissions includes { access = "read", path = "./out" } + */ + function _validateNoPayloadStorageSlots(address payload) internal view virtual { + string memory artifactPath = vm.getArtifactPathByDeployedCode(payload.code); + string memory artifact = vm.readFile(artifactPath); + + // vm.parseJson ABI-encodes the JSON value at the given key. + // A dynamic array is encoded as [uint256 offset][uint256 length][elements...], so + // decoding the first two words as (uint256, uint256) yields (offset, arrayLength). + bytes memory storageEncoded = vm.parseJson(artifact, '.storageLayout.storage'); + (, uint256 storageLength) = abi.decode(storageEncoded, (uint256, uint256)); + + require( + storageLength == 0, + string( + abi.encodePacked( + 'PAYLOAD_MUST_NOT_HAVE_STORAGE_VARIABLES: ', + artifactPath, + ' declares ', + Strings.toString(storageLength), + ' storage slot(s)' + ) + ) + ); + } } diff --git a/tests/ProtocolV2TestBase.t.sol b/tests/ProtocolV2TestBase.t.sol index 77df227a3..73bbe74c3 100644 --- a/tests/ProtocolV2TestBase.t.sol +++ b/tests/ProtocolV2TestBase.t.sol @@ -7,6 +7,7 @@ import {AaveV2Ethereum, AaveV2EthereumAssets} from 'aave-address-book/AaveV2Ethe import {AaveV2EthereumAMM} from 'aave-address-book/AaveV2EthereumAMM.sol'; import {IERC20} from 'openzeppelin-contracts/contracts/token/ERC20/IERC20.sol'; import {PayloadWithEmit} from './mocks/PayloadWithEmit.sol'; +import {PayloadWithStorage} from './mocks/PayloadWithStorage.sol'; contract ProtocolV2TestBaseTest is ProtocolV2TestBase { function setUp() public { @@ -40,3 +41,24 @@ contract ProtocolV2TestE2ETestAsset is ProtocolV2TestBase { defaultTest('AMMTEST', AaveV2EthereumAMM.POOL, address(new PayloadWithEmit()), false, false); } } + +contract ProtocolV2TestStorageValidation is ProtocolV2TestBase { + function test_noStorageSlots_passes() public { + // PayloadWithEmit has no state variables — should pass silently. + _validateNoPayloadStorageSlots(address(new PayloadWithEmit())); + } + + function test_withStorageSlots_reverts() public { + address payload = address(new PayloadWithStorage()); + // PayloadWithStorage declares `uint256 internal _randomStorageVariable` — must be rejected. + vm.expectRevert(); + _validateNoPayloadStorageSlots(payload); + } + + function test_unknownArtifact_logsWarning() public { + // makeAddr produces an address with no deployed code; getArtifactPathByDeployedCode + // cannot resolve it, so the function vm.getArtifactPathByDeployedCode reverts + vm.expectRevert(); + _validateNoPayloadStorageSlots(makeAddr('unknownPayload')); + } +} diff --git a/tests/ProtocolV3TestBase.t.sol b/tests/ProtocolV3TestBase.t.sol index e005b3d91..49ebb88c0 100644 --- a/tests/ProtocolV3TestBase.t.sol +++ b/tests/ProtocolV3TestBase.t.sol @@ -13,6 +13,7 @@ import {AaveV3MegaEth} from 'aave-address-book/AaveV3MegaEth.sol'; import {AaveV3Mantle} from 'aave-address-book/AaveV3Mantle.sol'; import {AaveV3Fantom} from 'aave-address-book/AaveV3Fantom.sol'; import {PayloadWithEmit} from './mocks/PayloadWithEmit.sol'; +import {PayloadWithStorage} from './mocks/PayloadWithStorage.sol'; contract ProtocolV3TestBaseTest is ProtocolV3TestBase { function setUp() public { @@ -179,4 +180,28 @@ contract ProtocolV3TestMantleSnapshot is ProtocolV3TestBase { false ); } + + // overriding the storage slot check as payload artifacts does not exists + function _validateNoPayloadStorageSlots(address payload) internal view override {} +} + +contract ProtocolV3TestStorageValidation is ProtocolV3TestBase { + function test_noStorageSlots_passes() public { + // PayloadWithEmit has no state variables — should pass silently. + _validateNoPayloadStorageSlots(address(new PayloadWithEmit())); + } + + function test_withStorageSlots_reverts() public { + address payload = address(new PayloadWithStorage()); + // PayloadWithStorage declares `uint256 internal _randomStorageVariable` — must be rejected. + vm.expectRevert(); + _validateNoPayloadStorageSlots(payload); + } + + function test_unknownArtifact_logsWarning() public { + // makeAddr produces an address with no deployed code; getArtifactPathByDeployedCode + // cannot resolve it, so the function vm.getArtifactPathByDeployedCode reverts + vm.expectRevert(); + _validateNoPayloadStorageSlots(makeAddr('unknownPayload')); + } } diff --git a/tests/mocks/PayloadWithStorage.sol b/tests/mocks/PayloadWithStorage.sol new file mode 100644 index 000000000..1f7a720d4 --- /dev/null +++ b/tests/mocks/PayloadWithStorage.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.10; + +import {IProposalGenericExecutor} from '../../src/interfaces/IProposalGenericExecutor.sol'; + +/** + * @dev Mock payload that incorrectly declares a state variable. + * Used to test that _validateNoPayloadStorageSlots detects storage variables. + */ +contract PayloadWithStorage is IProposalGenericExecutor { + uint256 internal _randomStorageVariable; + + function execute() external { + // do nothing just relax + } +}