OpenZeppelin defender fixes #211

minghinmatthewlam · 2023-12-21T23:49:41Z

Why this should be merged

Fixes #186

How this works

Handles fixes from the most recent report that are applicable:

missing docstrings
consistency of msg.sender vs _msgSender to support EIP2771 meta transactions
additional indexed parameters i.e. event UnlockTokens
lack of security contact
adding ReceiptReceived event
literal number with many digits
unused events
named return variables
write array length to stack for forloops to save gas
variables are initialized to default values and uses more gas i.e. uint256 val = 0;
_nativeMinter can be made constant

How this was tested

Unit + e2e test updates
CI

How is this documented

TODO

michaelkaplan13

👍 These are all great.

Left one super small nit, and one comment about how we'll need to update the E2E test that is failing.

tests/flows/basic_send_receive.go

michaelkaplan13 · 2023-12-22T14:40:44Z

tests/flows/basic_send_receive.go

+		deliveryReceipt.Logs,
+		subnetAInfo.TeleporterMessenger.ParseReceiptReceived)
+	Expect(err).Should(BeNil())
+	Expect(receiptEvent.MessageID).Should(Equal(expectedReceiptID))


I think the issue here is that, depending on the order that the tests are run in, the receipt queues have pre-existing state prior to a test starting. This makes it difficult to assert that a specific message will have a specific receipt at a given index.

2023-12-22T00:26:56.5997803Z <*big.Int | 0xc0001c3360>: {neg: false, abs: [4]} 2023-12-22T00:26:56.5998247Z to equal 2023-12-22T00:26:56.5999061Z <*big.Int | 0xc00044c7a0>: {neg: false, abs: [5]}�[0m 2023-12-22T00:26:56.6001273Z �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/home/runner/work/teleporter/teleporter/tests/flows/basic_send_receive.go:105�[0m �[38;5;243m@ 12/22/23 00:26:56.599�[0m 2023-12-22T00:26:56.6002668Z 2023-12-22T00:26:56.6003052Z �[38;5;9mFull Stack Trace�[0m 2023-12-22T00:26:56.6004080Z github.com/ava-labs/teleporter/tests/flows.BasicSendReceive({0x1edd8c0, 0xc0012f5c20}) 2023-12-22T00:26:56.6005544Z /home/runner/work/teleporter/teleporter/tests/flows/basic_send_receive.go:105 +0xac2 2023-12-22T00:26:56.6007069Z github.com/ava-labs/teleporter/tests/local.glob..func3.6() 2023-12-22T00:26:56.6008395Z /home/runner/work/teleporter/teleporter/tests/local/e2e_test.go:78 +0x27 2023-12-22T00:26:56.6010518Z < Exit �[1m[It]�[0m Send a message from Subnet A to Subnet B, and one from B to A �[38;5;243m- /home/runner/work/teleporter/teleporter/tests/local/e2e_test.go:77 @ 12/22/23 00:26:56.599 (7.104s)�[0m 2023-12-22T00:26:56.6014665Z �[38;5;9m• [FAILED] [7.104 seconds]�[0m 2023-12-22T00:26:56.6015121Z [Teleporter integration tests] 2023-12-22T00:26:56.6015971Z �[38;5;243m/home/runner/work/teleporter/teleporter/tests/local/e2e_test.go:58�[0m 2023-12-22T00:26:56.6017402Z �[38;5;9m�[1m[It] Send a message from Subnet A to Subnet B, and one from B to A�[0m 2023-12-22T00:26:56.6018549Z �[38;5;243m/home/runner/work/teleporter/teleporter/tests/local/e2e_test.go:77�[0m

We have a semi-hacky workaround for this in the send specific receipts test here, which involves sending messages to clear the receipt queues prior to proceeding with a test. This is only done in the case of a local network, which is ensure to have a reasonable receipt queue size.

added a check for the ReceiptReceived event emission

geoff-vball · 2023-12-27T14:37:33Z

contracts/src/CrossChainApplications/examples/NativeTokenBridge/NativeTokenDestination.sol

-    INativeMinter private immutable _nativeMinter =
+    INativeMinter public constant NATIVE_MINTER =


Definitely agree with the change to constant vs immutable here. Is there a gas-usage benefit the making it public?

no gas benefits, I'm going to change it back to private.

nvm updated back to public, to keep in convention with our other Warp precompile declarations

bernard-avalabs

LGTM

cam-schultz

LGTM, just one question about named return parameters.

cam-schultz · 2023-12-29T18:25:13Z

contracts/src/Teleporter/ReceiptQueue.sol

    }

    /**
     * @dev Returns the outstanding receipts for the given chain ID that should be included in the next message sent.
     */
    function getOutstandingReceiptsToSend(TeleporterMessageReceiptQueue storage queue)
        internal
-        returns (TeleporterMessageReceipt[] memory result)
+        returns (TeleporterMessageReceipt[] memory)


I agree that we should remove named return parameters for the sake of readability, but I'm curious if there are any gas benefits one way or the other? The OZ report does not specify.

from the gas reports didn't notice gas changes, think that makes sense since under the hood the named return parameters are likely the same as declaring a new local variable.

openzeppelin-code · 2024-01-02T20:05:35Z

OpenZeppelin defender fixes

Generated at commit: 49b61509783fc61e58a4cf775d012616428f5e38

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	0 0 0 5 19 24
Dependencies	Critical High Medium Low Note Total	0 0 0 1 34 35

For more details view the full report in OpenZeppelin Code Inspector

michaelkaplan13 · 2024-01-03T15:10:54Z

tests/flows/basic_send_receive.go

+	// This check is not performed for external networks because unrelated messages may have already changed
+	// the state of the receipt queues.
+	if !network.IsExternalNetwork() {
+		Expect(utils.CheckReceiptReceived(


In order to not depend on the order that tests run, I think we should clear the receipt queue at the start of this flow. Otherwise, other tests may send messages that cause the receipt queue length to be >5 at the start of this flow, and the first message sent back may not have this specific receipt.

moved the ClearReceiptQueue into a utils function and call in flow if not an external network

michaelkaplan13 · 2024-01-03T15:14:10Z

Is the OpenZeppelin submodule version used easy to update here to close this item?

michaelkaplan13 · 2024-01-03T15:16:53Z

I think this item is small enough to address in this PR also:

geoff-vball · 2024-01-03T15:57:50Z

@michaelkaplan13 our openzepplin submode is at version v4.8.1 and subnet-evm is using v4.9.3

michaelkaplan13 · 2024-01-03T16:17:20Z

@michaelkaplan13 our openzepplin submode is at version v4.8.1 and subnet-evm is using v4.9.3

Ah, thanks. I should have checked myself. I was assuming it was a more recent fix published for that issue.

Now I'm confused why that OpenZeppelin report item is listed at all then. Do we think it's a pure false positive?

geoff-vball · 2024-01-03T16:50:36Z

@michaelkaplan13 Actually, https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable is a separate repo. I don't see anywhere that a specific version is being included or even used anywhere though.

minghinmatthewlam · 2024-01-04T00:50:26Z

I think this item is small enough to address in this PR also:

only the first two apply, updated.

minghinmatthewlam · 2024-01-04T00:56:26Z

Is the OpenZeppelin submodule version used easy to update here to close this item?

@michaelkaplan13 this looks to be a false positive. If you click into the link attached in the alert GHSA-9c22-pwxw-p6hx, it specifies @openzeppelin/contracts and @openzeppelin/contracts-upgradeable. We only use the first package, and as @geoff-vball pointed out the versions we use are >v4.4.1

minghinmatthewlam requested review from michaelkaplan13, cam-schultz, gwen917, geoff-vball and bernard-avalabs as code owners December 21, 2023 23:49

minghinmatthewlam force-pushed the defender-fixes branch 3 times, most recently from d9b819d to 7a26993 Compare December 22, 2023 00:21

michaelkaplan13 reviewed Dec 22, 2023

View reviewed changes

minghinmatthewlam force-pushed the defender-fixes branch from 60589b6 to 3eab0a9 Compare December 22, 2023 20:23

minghinmatthewlam requested a review from michaelkaplan13 December 22, 2023 20:33

geoff-vball previously approved these changes Dec 27, 2023

View reviewed changes

bernard-avalabs previously approved these changes Dec 28, 2023

View reviewed changes

cam-schultz previously approved these changes Dec 29, 2023

View reviewed changes

minghinmatthewlam dismissed stale reviews from cam-schultz, bernard-avalabs, and geoff-vball via 68851a3 January 2, 2024 19:56

minghinmatthewlam force-pushed the defender-fixes branch from 68851a3 to 96ef29d Compare January 2, 2024 20:02

michaelkaplan13 reviewed Jan 3, 2024

View reviewed changes

openzeppelin defender fixes

b5042ae

minghinmatthewlam force-pushed the defender-fixes branch 2 times, most recently from 9000294 to af29ab6 Compare January 4, 2024 00:49

minghinmatthewlam force-pushed the defender-fixes branch from af29ab6 to f573d17 Compare January 4, 2024 00:51

minghinmatthewlam force-pushed the defender-fixes branch from f573d17 to 9266be3 Compare January 4, 2024 01:02

new alert fixes

afdc62a

minghinmatthewlam force-pushed the defender-fixes branch from 9266be3 to afdc62a Compare January 4, 2024 01:26

michaelkaplan13 previously approved these changes Jan 4, 2024

View reviewed changes

cam-schultz previously approved these changes Jan 4, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into defender-fixes

49b6150

minghinmatthewlam dismissed stale reviews from cam-schultz and michaelkaplan13 via 49b6150 January 4, 2024 22:00

minghinmatthewlam requested review from geoff-vball, michaelkaplan13, bernard-avalabs and cam-schultz January 4, 2024 22:00

geoff-vball approved these changes Jan 5, 2024

View reviewed changes

michaelkaplan13 approved these changes Jan 5, 2024

View reviewed changes

minghinmatthewlam merged commit c5547e3 into main Jan 5, 2024
15 checks passed

minghinmatthewlam deleted the defender-fixes branch January 5, 2024 15:37

minghinmatthewlam pushed a commit that referenced this pull request Oct 1, 2024

tests/utils/warpConfig: non-zero block timestamp (#211)

b787bc9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenZeppelin defender fixes #211

OpenZeppelin defender fixes #211

minghinmatthewlam commented Dec 21, 2023

michaelkaplan13 left a comment

michaelkaplan13 Dec 22, 2023

michaelkaplan13 Dec 22, 2023

minghinmatthewlam Dec 22, 2023

geoff-vball Dec 27, 2023

minghinmatthewlam Jan 4, 2024

minghinmatthewlam Jan 4, 2024 •

edited

Loading

bernard-avalabs left a comment

cam-schultz left a comment

cam-schultz Dec 29, 2023

minghinmatthewlam Jan 4, 2024

openzeppelin-code bot commented Jan 2, 2024 •

edited

Loading

michaelkaplan13 Jan 3, 2024

minghinmatthewlam Jan 4, 2024

michaelkaplan13 commented Jan 3, 2024

michaelkaplan13 commented Jan 3, 2024

geoff-vball commented Jan 3, 2024

michaelkaplan13 commented Jan 3, 2024

geoff-vball commented Jan 3, 2024

minghinmatthewlam commented Jan 4, 2024

minghinmatthewlam commented Jan 4, 2024 •

edited

Loading

		INativeMinter private immutable _nativeMinter =
		INativeMinter public constant NATIVE_MINTER =

OpenZeppelin defender fixes #211

OpenZeppelin defender fixes #211

Conversation

minghinmatthewlam commented Dec 21, 2023

Why this should be merged

How this works

How this was tested

How is this documented

michaelkaplan13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minghinmatthewlam Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

bernard-avalabs left a comment

Choose a reason for hiding this comment

cam-schultz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openzeppelin-code bot commented Jan 2, 2024 • edited Loading

OpenZeppelin defender fixes

🚨 Report Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkaplan13 commented Jan 3, 2024

michaelkaplan13 commented Jan 3, 2024

geoff-vball commented Jan 3, 2024

michaelkaplan13 commented Jan 3, 2024

geoff-vball commented Jan 3, 2024

minghinmatthewlam commented Jan 4, 2024

minghinmatthewlam commented Jan 4, 2024 • edited Loading

minghinmatthewlam Jan 4, 2024 •

edited

Loading

openzeppelin-code bot commented Jan 2, 2024 •

edited

Loading

minghinmatthewlam commented Jan 4, 2024 •

edited

Loading