Skip to content
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.

Sign up for GitHub

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

check msg.data length #13352

Merged
merged 3 commits into from
May 31, 2024
Merged

check msg.data length #13352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23df82a
check msg.data length
shileiwill May 29, 2024
15a8ef8
Merge 23df82a07d308c83324cc8ddcaac473aa72bb7e8 into 4c7e5a0efa90aed5d…
shileiwill May 30, 2024
ee0c8fe
Update gethwrappers
app-token-issuer-infra-releng[bot] May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/breezy-pears-cough.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

check data size #bugfix
5 changes: 5 additions & 0 deletions contracts/.changeset/smart-trainers-begin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@chainlink/contracts": patch
---

check data size #bugfix
1 change: 1 addition & 0 deletions contracts/scripts/native_solc_compile_all_automation
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,5 +103,6 @@ compileContract automation/dev/v2_3/AutomationRegistrar2_3.sol
compileContract automation/dev/v2_3/AutomationRegistry2_3.sol
compileContract automation/dev/v2_3/AutomationRegistryLogicA2_3.sol
compileContract automation/dev/v2_3/AutomationRegistryLogicB2_3.sol
compileContract automation/dev/v2_3/AutomationRegistryLogicC2_3.sol
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 crazy we've made it this far without having to add this lol

compileContract automation/dev/v2_3/AutomationUtils2_3.sol
compileContract automation/dev/interfaces/v2_3/IAutomationRegistryMaster2_3.sol
37 changes: 37 additions & 0 deletions contracts/src/v0.8/automation/dev/test/AutomationRegistry2_3.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1611,6 +1611,43 @@ contract BillingOverrides is SetUp {
}

contract Transmit is SetUp {
function test_transmitRevertWithExtraBytes() external {
bytes32[3] memory exampleReportContext = [
bytes32(0x1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef),
bytes32(0xabcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890),
bytes32(0x7890abcdef1234567890abcdef1234567890abcdef1234567890abcdef123456)
Comment on lines +1616 to +1618
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

total nitpick, but it might be a little cleaner if we just use the _random() function to generate these bytes ex...

bytes32[3] memory exampleReportContext = [_random(), _random(), _random()];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! I will take a note for this. will leave it as is for now since dont want tobother to get approvals again.

];

bytes memory exampleRawReport = hex"deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef";

bytes32[] memory exampleRs = new bytes32[](3);
exampleRs[0] = bytes32(0x1234561234561234561234561234561234561234561234561234561234561234);
exampleRs[1] = bytes32(0x1234561234561234561234561234561234561234561234561234561234561234);
exampleRs[2] = bytes32(0x7890789078907890789078907890789078907890789078907890789078907890);

bytes32[] memory exampleSs = new bytes32[](3);
exampleSs[0] = bytes32(0x1234561234561234561234561234561234561234561234561234561234561234);
exampleSs[1] = bytes32(0x1234561234561234561234561234561234561234561234561234561234561234);
exampleSs[2] = bytes32(0x1234561234561234561234561234561234561234561234561234561234561234);

bytes32 exampleRawVs = bytes32(0x1234561234561234561234561234561234561234561234561234561234561234);

bytes memory transmitData = abi.encodeWithSelector(
registry.transmit.selector,
exampleReportContext,
exampleRawReport,
exampleRs,
exampleSs,
exampleRawVs
);
bytes memory badTransmitData = bytes.concat(transmitData, bytes1(0x00)); // add extra data
vm.startPrank(TRANSMITTERS[0]);
(bool success, bytes memory returnData) = address(registry).call(badTransmitData); // send the bogus transmit
assertFalse(success, "Call did not revert as expected");
assertEq(returnData, abi.encodePacked(Registry.InvalidDataLength.selector));
Comment on lines +1645 to +1647
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize we wouldn't be able to use the expectRevert prank, but this seems like a very reasonable alternative! Nicely done 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, i also prefer the shorter expectRevert, but expectRevert doesnt work for low level calls, which we are doing here.

vm.stopPrank();
}

function test_handlesMixedBatchOfBillingTokens() external {
uint256[] memory prevUpkeepBalances = new uint256[](3);
prevUpkeepBalances[0] = registry.getBalance(linkUpkeepID);
Expand Down
5 changes: 5 additions & 0 deletions contracts/src/v0.8/automation/dev/v2_3/AutomationRegistry2_3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ contract AutomationRegistry2_3 is AutomationRegistryBase2_3, OCR2Abstract, Chain
bytes32 rawVs
) external override {
uint256 gasOverhead = gasleft();
// use this msg.data length check to ensure no extra data is included in the call
// 4 is first 4 bytes of the keccak-256 hash of the function signature. ss.length == rs.length so use one of them
// 4 + (32 * 3) + (rawReport.length + 32 + 32) + (32 * rs.length + 32 + 32) + (32 * ss.length + 32 + 32) + 32
uint256 requiredLength = 324 + rawReport.length + 64 * rs.length;
Comment on lines +93 to +96
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing 🎉 I'm so glad there was a minimally complex solution to this problem

if (msg.data.length != requiredLength) revert InvalidDataLength();
HotVars memory hotVars = s_hotVars;

if (hotVars.paused) revert RegistryPaused();
Expand Down
2 changes: 1 addition & 1 deletion ...gethwrappers/generated/automation_registry_wrapper_2_3/automation_registry_wrapper_2_3.go
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion core/gethwrappers/generation/generated-wrapper-dependency-versions-do-not-edit.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ automation_registry_logic_a_wrapper_2_2: ../../contracts/solc/v0.8.19/Automation
automation_registry_logic_a_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_3/AutomationRegistryLogicA2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_3/AutomationRegistryLogicA2_3.bin 73b5cc3ece642abbf6f2a4c9188335b71404f4dd0ad10b761390b6397af6f1c8
automation_registry_logic_b_wrapper_2_2: ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_2/AutomationRegistryLogicB2_2.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_2/AutomationRegistryLogicB2_2.bin a6d33dfbbfb0ff253eb59a51f4f6d6d4c22ea5ec95aae52d25d49a312b37a22f
automation_registry_logic_b_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.bin fbf6f6cf4e6858855ff5da847c3baa4859dd997cfae51f2fa0651e4fa15b92c9
automation_registry_logic_c_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistryLogicC2_3/AutomationRegistryLogicC2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicC2_3/AutomationRegistryLogicC2_3.bin 3ee51aa2f946b9fe3583b4a8526d29721339f96774e410bd37ddfe8184a63701
automation_registry_wrapper_2_2: ../../contracts/solc/v0.8.19/AutomationRegistry2_2/AutomationRegistry2_2.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_2/AutomationRegistry2_2.bin de60f69878e9b32a291a001c91fc8636544c2cfbd9b507c8c1a4873b602bfb62
automation_registry_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.bin 10078161924b38cf968ceb65f54078412832ada9abeebcd011ee7291811921c2
automation_registry_wrapper_2_3: ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.bin f8f920a225fdb1e36948dd95bae3aa46ecc2b01fd113480e111960b5e5f95624
automation_utils_2_1: ../../contracts/solc/v0.8.16/AutomationUtils2_1/AutomationUtils2_1.abi ../../contracts/solc/v0.8.16/AutomationUtils2_1/AutomationUtils2_1.bin 815b17b63f15d26a0274b962eefad98cdee4ec897ead58688bbb8e2470e585f5
automation_utils_2_2: ../../contracts/solc/v0.8.19/AutomationUtils2_2/AutomationUtils2_2.abi ../../contracts/solc/v0.8.19/AutomationUtils2_2/AutomationUtils2_2.bin 8743f6231aaefa3f2a0b2d484258070d506e2d0860690e66890dccc3949edb2e
automation_utils_2_3: ../../contracts/solc/v0.8.19/AutomationUtils2_3/AutomationUtils2_3.abi ../../contracts/solc/v0.8.19/AutomationUtils2_3/AutomationUtils2_3.bin 11e2b481dc9a4d936e3443345d45d2cc571164459d214917b42a8054b295393b
Expand Down
1 change: 1 addition & 0 deletions core/gethwrappers/go_generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ package gethwrappers
//go:generate go run ./generation/generate/wrap.go ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistry2_3/AutomationRegistry2_3.bin AutomationRegistry automation_registry_wrapper_2_3
//go:generate go run ./generation/generate/wrap.go ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_3/AutomationRegistryLogicA2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicA2_3/AutomationRegistryLogicA2_3.bin AutomationRegistryLogicA automation_registry_logic_a_wrapper_2_3
//go:generate go run ./generation/generate/wrap.go ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicB2_3/AutomationRegistryLogicB2_3.bin AutomationRegistryLogicB automation_registry_logic_b_wrapper_2_3
//go:generate go run ./generation/generate/wrap.go ../../contracts/solc/v0.8.19/AutomationRegistryLogicC2_3/AutomationRegistryLogicC2_3.abi ../../contracts/solc/v0.8.19/AutomationRegistryLogicC2_3/AutomationRegistryLogicC2_3.bin AutomationRegistryLogicC automation_registry_logic_c_wrapper_2_3
//go:generate go run ./generation/generate/wrap.go ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.abi ../../contracts/solc/v0.8.19/IAutomationRegistryMaster2_3/IAutomationRegistryMaster2_3.bin IAutomationRegistryMaster2_3 i_automation_registry_master_wrapper_2_3
//go:generate go run ./generation/generate/wrap.go ../../contracts/solc/v0.8.19/AutomationUtils2_3/AutomationUtils2_3.abi ../../contracts/solc/v0.8.19/AutomationUtils2_3/AutomationUtils2_3.bin AutomationUtils automation_utils_2_3
//go:generate go run ./generation/generate/wrap.go ../../contracts/solc/v0.8.19/ArbitrumModule/ArbitrumModule.abi ../../contracts/solc/v0.8.19/ArbitrumModule/ArbitrumModule.bin ArbitrumModule arbitrum_module
Expand Down
Loading