-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(invariants
): sample typed storage values
#11204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
forge
): sample typed storage valuesinvariants
): sample typed storage values
🚀 this is huge improvement, in same line maybe you could look after into adding same for state diff decoding #9504 should be quite similar CC @sakulstra |
Yeah, I'll look into that next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good so far, spotted one issue with matching storage layout based on contract identifier. Unfortunately this doesn't catch ds chief bug below because it cannot handle mappings (left a comment re this too, maybe we could think at a way for complex types?). Also note that for this to be taken into account during invariant campaigns one should set extra output in project configs
extra_output = ["storageLayout"]
source contract src/SimpleDSChief.sol
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
contract SimpleDSChief {
mapping(bytes32 => address) public slates;
mapping(address => bytes32) public votes;
mapping(address => uint256) public approvals;
mapping(address => uint256) public deposits;
bool public hacked = false;
function lock(uint256 wad) public {
deposits[msg.sender] = add(deposits[msg.sender], wad);
addWeight(wad, votes[msg.sender]);
}
function free(uint256 wad) public {
deposits[msg.sender] = sub(deposits[msg.sender], wad);
subWeight(wad, votes[msg.sender]);
}
function voteYays(address yay) public {
bytes32 slate = keccak256(abi.encodePacked(yay));
slates[slate] = yay;
voteSlate(slate);
}
function etch(address yay) public {
bytes32 hash = keccak256(abi.encodePacked(yay));
slates[hash] = yay;
}
function voteSlate(bytes32 slate) public {
uint256 weight = deposits[msg.sender];
subWeight(weight, votes[msg.sender]);
votes[msg.sender] = slate;
addWeight(weight, votes[msg.sender]);
}
function addWeight(uint256 weight, bytes32 slate) internal {
address yay = slates[slate];
approvals[yay] = add(approvals[yay], weight);
}
function subWeight(uint256 weight, bytes32 slate) internal {
address yay = slates[slate];
approvals[yay] = sub(approvals[yay], weight);
}
function add(uint256 x, uint256 y) internal pure returns (uint256 z) {
require((z = x + y) >= x);
}
function sub(uint256 x, uint256 y) internal pure returns (uint256 z) {
require((z = x - y) <= x);
}
function checkInvariant() public {
bytes32 senderSlate = votes[msg.sender];
address option = slates[senderSlate];
uint256 senderDeposit = deposits[msg.sender];
if (approvals[option] < senderDeposit) {
hacked = true;
}
}
}
test contract test/SimpleDSChiefTest.t.sol
:
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import {Test} from "forge-std/Test.sol";
import {SimpleDSChief} from "src/SimpleDSChief.sol";
contract SimpleDSChiefTest is Test {
SimpleDSChief dsChief;
function setUp() public {
dsChief = new SimpleDSChief();
targetContract(address(dsChief));
targetSender(address(0x10000));
targetSender(address(0x20000));
targetSender(address(0x30000));
}
/// forge-config: default.invariant.runs = 1000
function invariant_check_dschief() public view {
assertFalse(dsChief.hacked());
}
}
// Try to determine the type of this storage slot | ||
let storage_type = storage_layout.and_then(|layout| { | ||
// Find the storage entry for this slot | ||
layout.storage.iter().find(|s| s.slot == storage_slot.to_string()).and_then(|storage| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be great instead s.slot == storage_slot.to_string()
to also figure out if it falls into a mapping base slot and what's the type (this logic could then be reused for decoding in #9504 as well) though not trivial and not sure exactly how achievable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, looking into how we can make this work for mappings with simpler types first such as mapping(address => bytes32)
. We can later look into complex mappings with custom structs such as mapping(address => CustomStruct)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, yep, simple mappings support would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some cleanup and optimizations to do but pls check: c7029d5.
Found the violation using the above:
[FAIL: assertion failed]
[Sequence] (original: 333, shrunk: 6)
sender=0x0000000000000000000000000000000000020000 addr=[src/SimpleDSChief.sol:SimpleDSChief]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=lock(uint256) args=[48459496635163284513498257876386846003409259726038080939842009107832010600766 [4.845e76]]
sender=0x0000000000000000000000000000000000030000 addr=[src/SimpleDSChief.sol:SimpleDSChief]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=voteSlate(bytes32) args=[0xe169a59fb923f91b8fb1d1fc9258a999c5a19ad050199cd59889d6fa30d99753]
sender=0x0000000000000000000000000000000000030000 addr=[src/SimpleDSChief.sol:SimpleDSChief]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=lock(uint256) args=[3065]
sender=0x0000000000000000000000000000000000020000 addr=[src/SimpleDSChief.sol:SimpleDSChief]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=voteYays(address) args=[0x0000000000000000000000000000000000001474]
sender=0x0000000000000000000000000000000000030000 addr=[src/SimpleDSChief.sol:SimpleDSChief]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=voteYays(address) args=[0x078B931Fa9ccaAF6580B28Eec0E2fA94D9E0cB0F]
sender=0x0000000000000000000000000000000000020000 addr=[src/SimpleDSChief.sol:SimpleDSChief]0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f calldata=checkInvariant() args=[]
invariant_check_dschief() (runs: 577, calls: 288500, reverts: 21306)
Failure call sequence
function testViotationSequence2() public {
dsChief = new SimpleDSChief();
address sender200 = address(0x20000);
address sender300 = address(0x30000);
vm.prank(sender200);
dsChief.lock(
48459496635163284513498257876386846003409259726038080939842009107832010600766
);
vm.startPrank(sender300);
dsChief.voteSlate(
0xe169a59fb923f91b8fb1d1fc9258a999c5a19ad050199cd59889d6fa30d99753
);
dsChief.lock(3065);
vm.stopPrank();
vm.prank(sender200);
dsChief.voteYays(0x0000000000000000000000000000000000001474);
vm.prank(sender300);
dsChief.voteYays(0x078B931Fa9ccaAF6580B28Eec0E2fA94D9E0cB0F);
vm.prank(sender200);
dsChief.checkInvariant();
// Invariant broken.
assertTrue(dsChief.hacked());
}
"call_sequence": [
{
"sender": "0x0000000000000000000000000000000000020000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0xdd4670646b231a65c89caadb908c2e1634448f2b534d25b8a37ded97e5e06bfec772713e",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "lock",
"signature": "lock(uint256)",
"args": "48459496635163284513498257876386846003409259726038080939842009107832010600766 \u001b[2m[4.845e76]\u001b[0m",
"raw_args": "48459496635163284513498257876386846003409259726038080939842009107832010600766"
},
{
"sender": "0x0000000000000000000000000000000000030000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0xed337208e169a59fb923f91b8fb1d1fc9258a999c5a19ad050199cd59889d6fa30d99753",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "voteSlate",
"signature": "voteSlate(bytes32)",
"args": "0xe169a59fb923f91b8fb1d1fc9258a999c5a19ad050199cd59889d6fa30d99753",
"raw_args": "0xe169a59fb923f91b8fb1d1fc9258a999c5a19ad050199cd59889d6fa30d99753"
},
{
"sender": "0x0000000000000000000000000000000000030000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0xdd4670640000000000000000000000000000000000000000000000000000000000000bf9",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "lock",
"signature": "lock(uint256)",
"args": "3065",
"raw_args": "3065"
},
{
"sender": "0x0000000000000000000000000000000000020000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0x30d6c5750000000000000000000000000000000000000000000000000000000000001474",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "voteYays",
"signature": "voteYays(address)",
"args": "0x0000000000000000000000000000000000001474",
"raw_args": "0x0000000000000000000000000000000000001474"
},
{
"sender": "0x0000000000000000000000000000000000030000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0x30d6c575000000000000000000000000078b931fa9ccaaf6580b28eec0e2fa94d9e0cb0f",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "voteYays",
"signature": "voteYays(address)",
"args": "0x078B931Fa9ccaAF6580B28Eec0E2fA94D9E0cB0F",
"raw_args": "0x078B931Fa9ccaAF6580B28Eec0E2fA94D9E0cB0F"
},
{
"sender": "0x0000000000000000000000000000000000020000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0xe79487da",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "checkInvariant",
"signature": "checkInvariant()",
"args": "",
"raw_args": ""
}
]
Another failure sequence
function testViolationSequence() public {
dsChief = new SimpleDSChief();
address sender = address(0x20000);
vm.startPrank(sender);
uint256 lockAmt = 97755186804595591533339148550949742259098835763;
dsChief.lock(lockAmt);
bytes32 slate = 0xcc53d03fda58d9a098fd16c62dc974b0bc1c6e942fad55724f240f085e67485c;
dsChief.voteSlate(slate);
address etchAddr = 0x000000000000000000000000000000000000178c;
dsChief.etch(etchAddr);
dsChief.checkInvariant();
// Invariant broken.
assertTrue(dsChief.hacked());
}
"call_sequence": [
{
"sender": "0x0000000000000000000000000000000000020000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0xdd467064000000000000000000000000111f7e82a185dd2d1ee7350ff4b5ceaece1b5b33",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "lock",
"signature": "lock(uint256)",
"args": "97755186804595591533339148550949742259098835763 \u001b[2m[9.775e46]\u001b[0m",
"raw_args": "97755186804595591533339148550949742259098835763"
},
{
"sender": "0x0000000000000000000000000000000000020000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0xed337208cc53d03fda58d9a098fd16c62dc974b0bc1c6e942fad55724f240f085e67485c",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "voteSlate",
"signature": "voteSlate(bytes32)",
"args": "0xcc53d03fda58d9a098fd16c62dc974b0bc1c6e942fad55724f240f085e67485c",
"raw_args": "0xcc53d03fda58d9a098fd16c62dc974b0bc1c6e942fad55724f240f085e67485c"
},
{
"sender": "0x0000000000000000000000000000000000020000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0x77c243eb000000000000000000000000000000000000000000000000000000000000178c",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "etch",
"signature": "etch(address)",
"args": "0x000000000000000000000000000000000000178c",
"raw_args": "0x000000000000000000000000000000000000178c"
},
{
"sender": "0x0000000000000000000000000000000000020000",
"addr": "0x5615deb798bb3e4dfa0139dfa1b3d433cc23b72f",
"calldata": "0xe79487da",
"contract_name": "src/SimpleDSChief.sol:SimpleDSChief",
"func_name": "checkInvariant",
"signature": "checkInvariant()",
"args": "",
"raw_args": ""
}
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unable to reproduce the violations reliably likely because we're still not checking whether the storage_slot
belongs to the mapping or not but instead optmistically inserting all storage_values
that can be decoded into any of the mapping value types. This is still better than not handling mapping storage values but isn't ideal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, definitely better, maybe we insert only values for the known mapping here
https://github.com/foundry-rs/foundry/pull/11204/files#diff-c5b4765d032eafa6ae80b980dbf25e2ad41b34d65c33fff247b5bd9bbd333db5R369-R374
so if we don't have a mapping with bool for example then we don't insert (in DS chief sample we only have
mapping(bytes32 => address) public slates;
mapping(address => bytes32) public votes;
mapping(address => uint256) public approvals;
mapping(address => uint256) public deposits;
so we insert only as address, bytes32 and uint256).
For tests - you could also set smth like timeout = 3600
in invariant config section so you won't be bounded by the number of runs but rather let it run until 3600 seconds expires
Motivation
Closes #8116
Solution
StorageLayout
toContractData
andTargetedContracts
state_changeset
, theStorageLayout
is looked up to decide whether to insert it as a typed sample value OR raw value.storageLayout
usingextra_output = ["storageLayout"]
infoundry.toml
OR use the cli flag--extra-output storageLayout
PR Checklist