OpenZeppelin Audit Cleanup Items #106

michaelkaplan13 · 2023-11-03T21:58:19Z

Why this should be merged

Addresses clean up items from the first version of the OpenZeppelin audit report (to be published at a later date).

How this works

Sets the visibility level to private for non-constant and non-immutable state variables in abstract contracts to avoid inheriting contracts from being able to override their behavior.
Adds docstrings to all structs.
Adds security contact link to contract source code.
Removes unused state variables.
Adds named parameters of mappings.
Removes unused named return variables in favor of explicitly returned values.
Uses explicit imports for contracts under Teleporter/
Makes relayerRewardAddresses and relayerRewardAmounts mappings internal to avoid redundant getter functions. Preferable than removing the explicit getter functions which are defined in the interface.
Fixes typos.
Adds getNextMessageID to the ITeleporterMessenger interface.
Updates documentation comments of SafeERC20TransferFrom
Removes unused external checkIsAllowedRelayer function.
Uses Math library for min calculation in receipt array sizes

How this was tested

Unit tests and CI.

How is this documented

N/A

…lin-audit-cleanup-items

minghinmatthewlam · 2023-11-03T22:37:30Z

contracts/src/Teleporter/ITeleporterMessenger.sol

+// A message receipt identifies the message ID that was
+// delivered and address that should be able to redeem the
+// reward for that message.


Suggested change

// A message receipt identifies the message ID that was

// delivered and address that should be able to redeem the

// reward for that message.

// A message receipt identifies the message ID that was

// delivered and the address that can redeem the

// rewards for that message.

Nice catch. I think singular "reward" is correct in this case, but fixed the other.

minghinmatthewlam · 2023-11-03T22:40:10Z

contracts/src/Teleporter/ITeleporterMessenger.sol

+// to be sent to the given destination chain ID and address. Includes the fee
+// information for the message, the amount of gas the relayer must provide to execute
+// the message on the destination chain, the relayer accounts allowed to deliver the
+// message, and the message data itself.


one of the points in the audit points out we only use this struct as a parameter, do we want to remove and just pass in arguments to functions?

I see you changed the _sendTeleporterMessage input to be TeleporterMessageInput, but thoughts on removing it as requirement for the interface, and moving to struct to TeleporterMessenger? Not a big deal either way imo

The use of the TeleporterMessageInput struct is actually needed in order to reduce the depth of the stack in both _sendTeleporterMessage and sendTeleporterMessage.

contracts/src/Teleporter/ITeleporterMessenger.sol

minghinmatthewlam · 2023-11-03T22:44:13Z

contracts/src/Teleporter/ReceiptQueue.sol

@@ -5,18 +5,26 @@

 pragma solidity 0.8.18;

-import "./ITeleporterMessenger.sol";
+import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";


there is a rule in solhint.json for no-global-imports we can remove, but would require us to handle explicit imports in the cross chain apps as well, and probably ignore linting the tests folders.

Looking into whether we can have lint rules different between directories/files.

@minghinmatthewlam We could ignore the tests folder for the current solhint run, and give the tests folder it's own solhint.json and check.

I'm not opposed to enabling the no-global-imports linting rule and correcting everywhere needed now. It will put us in the best position moving forward I think.

Updated now. I think having different linting rules for different directories would likely cause more confusion that benefits.

contracts/src/Teleporter/ReceiptQueue.sol

minghinmatthewlam · 2023-11-03T23:09:37Z

contracts/src/Teleporter/TeleporterMessenger.sol

@@ -287,7 +282,7 @@ contract TeleporterMessenger is ITeleporterMessenger, ReentrancyGuards {
        // Check the message has not been delivered before by checking that there is no relayer reward
        // address stored for it already.
        require(
-            relayerRewardAddresses[warpMessage.sourceChainID][
+            _relayerRewardAddresses[warpMessage.sourceChainID][


I think instead of checking _relayerRewardAddresses directly here, we should make an internal _messageReceived and use that check here to make it more clear.

That wasn't an item called out in the audit, so I think it's out of scope for this PR.

I think it falls under L-08 Semantic Overload for relayerRewardAddress

Gotcha, I personally am not super inclined to make the change given it increases overall contract size and gas costs and the check is properly described in the comment directly above here. If others feel strongly though I'm open to it.

When you say gas costs do you mean the increase in gas for deployment, or are you expecting an increase in gas for wrappign the check now in a function call?

Either way think the gas cost is minor in favor of more readable code so would be my vote.

Fair enough, yeah I checked the forge test --gas-report before and after making those change, and it was a very minor increase.

I added an internal _messageReceived now.

contracts/src/Teleporter/TeleporterMessenger.sol

minghinmatthewlam · 2023-11-03T23:18:12Z

contracts/src/Teleporter/tests/CheckAllowedRelayerTests.t.sol

-        TeleporterMessengerTest.setUp();
-    }
-
+contract CheckIsAllowedRelayerTest is TeleporterMessenger, Test {


do we want to test internal functions of TeleproterMessenger by making derived contracts? I would lean towards _checkIsAllowedRealyer being tested in TeleporterMessengerTests

What's the motivation for not wanting it to be in its own test suite? It makes it easier to find and isolate IMO.

I don't love making a derived contract to test internal functions, but I think it's the easy option that provides the needed coverage.

Good with the derived contract and own test suite, just want to make sure consistent with how other internal functions were tested.

This was actually the first internal method in the TeleporterMessenger contract. 👍

They theoretically could be tested through various inputs to the public/external methods that call them, but in this case I think I'd rather keep the isolated test coverage for it specifically.

minghinmatthewlam · 2023-11-03T23:24:52Z

contracts/src/Teleporter/upgrades/TeleporterUpgradeable.sol

@@ -5,7 +5,7 @@

 pragma solidity 0.8.18;

-import "./TeleporterRegistry.sol";
+import {TeleporterRegistry} from "./TeleporterRegistry.sol";

 /**
 * @dev TeleporterUpgradeable provides upgrade utility for applications built on top


can't comment on line, but confirming for minTeleporterVersion we can't change visbility, since derived contracts will need to implement updateMinTeleporterVersion and have access to the state variable. Trying to think how we can only allow updating in updateMinTeleporterVersion

Perhaps we should add a comment at that state variable explaining this special case and why it diverges from Solidity best practices.

Good call. Adding now

contracts/src/WarpProtocolRegistry.sol

contracts/src/CrossChainApplications/ExampleMessenger/ExampleCrossChainMessenger.sol

contracts/src/Teleporter/ITeleporterMessenger.sol

geoff-vball · 2023-11-06T15:06:13Z

contracts/src/Teleporter/ReceiptQueue.sol

@@ -5,18 +5,26 @@

 pragma solidity 0.8.18;

-import "./ITeleporterMessenger.sol";
+import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";


@minghinmatthewlam We could ignore the tests folder for the current solhint run, and give the tests folder it's own solhint.json and check.

contracts/src/Teleporter/TeleporterMessenger.sol

Co-authored-by: minghinmatthewlam <matthew.lam@avalabs.org> Signed-off-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com>

…bs/teleporter into open-zeppelin-audit-cleanup-items

…lin-audit-cleanup-items

cam-schultz

A couple of small comments, including an amendment to a suggestion I previously made.

cam-schultz · 2023-11-08T16:15:19Z

scripts/constants.sh

-# Use lower_case variables in the scripts and UPPER_CASE variables for override
-# Use the constants.sh for env overrides
-
-TELEPORTER_PATH=$(


What's the reason for removing this and putting it in the other scripts? My thinking was that we could put this here and source constants.sh everywhere that needs it.

That was my originally thinking also, but then I realized having TELEPORTER_PATH set in each individual script is consistent with the pattern used in the subnet-evm scripts.

The reason is that sourcing the constants.sh file from a script requires knowing where the script is in relation to to the current working directory the script is being called from. So it's less error prone to set the relative path to the base of the repo from each script individual (no matter where it's called from), and then source constants.sh relative from there.

Pesky chickens and eggs 😆 Makes sense

cam-schultz · 2023-11-08T16:15:36Z

scripts/local/e2e_test.sh

+#
+# We use "export" here instead of just setting a bash variable because we need
+# to pass this flag to all child processes spawned by the shell.
+export CGO_CFLAGS="-O -D__BLST_PORTABLE__"


Does this still need to be here now that it's included in constants.sh?

Nope, thanks for catching.

contracts/src/Teleporter/TeleporterMessenger.sol

Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com> Signed-off-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com>

cam-schultz

LGTM!

minghinmatthewlam · 2023-11-08T22:21:26Z

.github/workflows/test.yml

@@ -9,7 +9,7 @@ on:
      - "*"

 env:
-  RELAYER_VERSION: v0.2.2
+  RELAYER_VERSION: v0.2.3


can we also update the other references to relayer version v0.2.2. in docker run/test

Good catch 👍

minghinmatthewlam

LGTM 👍

bernard-avalabs

LGTM

bernard-avalabs · 2023-11-09T04:36:45Z

contracts/src/Teleporter/TeleporterMessenger.sol

        // case where there is no fee to be paid for the given message.
        if (messageInfo.messageHash == bytes32(0)) {
            return;
        }

-        // Delete the message information from state now that we know it has been delivered.
+        // Delete the message information from state now that it is known to be delivered.


nit: "it is known to be" seems a bit awkward. Can we just say "... now that the message has been delivered"?

I'd prefer to leave it as because technically the fact that the message is delivered isn't enough to delete it from state here. The sending chain has to be informed of the delivery via a receipt.

bernard-avalabs · 2023-11-09T04:38:16Z

contracts/src/Teleporter/upgrades/TeleporterUpgradeable.sol

+
+    /**
+     * @dev The minimum required Teleporter version that the contract is allowed
+     * to receive messages from. Should only be update through the `updateMinTeleporterVersion`


nit: update -> updated

michaelkaplan13 added 3 commits November 3, 2023 13:17

OpenZeppelin audit clean up items

43d4c72

Add named values for mappings

c2f4325

Update ABI bindings

ba5ca82

michaelkaplan13 marked this pull request as ready for review November 3, 2023 21:58

michaelkaplan13 requested review from cam-schultz, minghinmatthewlam, gwen917, geoff-vball, bernard-avalabs and feuGeneA as code owners November 3, 2023 21:58

Merge branch 'main' of github.com:ava-labs/teleporter into open-zeppe…

5c78b97

…lin-audit-cleanup-items

minghinmatthewlam reviewed Nov 3, 2023

View reviewed changes

michaelkaplan13 added 4 commits November 6, 2023 10:18

Receipt comment wording

8a37fe8

Remove unnecessary security contact comment

cfd432a

No global imports

a775ce8

Update receipt queue comment

c660fae

geoff-vball reviewed Nov 6, 2023

View reviewed changes

michaelkaplan13 and others added 12 commits November 6, 2023 11:31

Update safeTransferFrom comment

f2aef0c

Update comment wording

16781ad

Update contracts/src/Teleporter/TeleporterMessenger.sol

7f78823

Co-authored-by: minghinmatthewlam <matthew.lam@avalabs.org> Signed-off-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com>

Update relayer reward address comment

7d7f685

Update contracts/src/Teleporter/TeleporterMessenger.sol

8d4c6a6

Co-authored-by: minghinmatthewlam <matthew.lam@avalabs.org> Signed-off-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com>

Update interface comments

228e5dd

Merge branch 'open-zeppelin-audit-cleanup-items' of github.com:ava-la…

38bdd13

…bs/teleporter into open-zeppelin-audit-cleanup-items

Remove all named return values

43f9ddc

Merged latest main branch

588f585

Add internal _messageReceived. Fixed BLST CGO flag

28af002

Update ABI bindings. Fix linter

5075d5a

Merge branch 'main' of github.com:ava-labs/teleporter into open-zeppe…

0c0849d

…lin-audit-cleanup-items

geoff-vball mentioned this pull request Nov 6, 2023

Set blst to portable mode for abi check #115

Closed

michaelkaplan13 added 2 commits November 7, 2023 16:17

Move CGO flags to constants.sh

c57d5b1

Merged latest commits from main

d4d9d4b

michaelkaplan13 requested review from geoff-vball, minghinmatthewlam, bernard-avalabs and cam-schultz November 7, 2023 21:31

cam-schultz reviewed Nov 8, 2023

View reviewed changes

michaelkaplan13 and others added 2 commits November 8, 2023 12:46

Remove unnecessary CGO flags

a393be7

Update contracts/src/Teleporter/TeleporterMessenger.sol

9c587d8

Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com> Signed-off-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com>

michaelkaplan13 requested a review from cam-schultz November 8, 2023 19:34

cam-schultz previously approved these changes Nov 8, 2023

View reviewed changes

Bump relayer version

f1f24a5

michaelkaplan13 dismissed cam-schultz’s stale review via f1f24a5 November 8, 2023 21:55

minghinmatthewlam reviewed Nov 8, 2023

View reviewed changes

michaelkaplan13 added 3 commits November 8, 2023 17:25

Source constants.sh from run_setup.sh

cd8b08c

Bump relayer version in docker-compose files

793289e

Source constant.sh from workflow

af24889

minghinmatthewlam previously approved these changes Nov 8, 2023

View reviewed changes

cam-schultz previously approved these changes Nov 8, 2023

View reviewed changes

bernard-avalabs previously approved these changes Nov 9, 2023

View reviewed changes

Fix comment tense

9060707

michaelkaplan13 dismissed stale reviews from bernard-avalabs, cam-schultz, and minghinmatthewlam via 9060707 November 9, 2023 14:27

cam-schultz approved these changes Nov 9, 2023

View reviewed changes

minghinmatthewlam approved these changes Nov 9, 2023

View reviewed changes

bernard-avalabs approved these changes Nov 9, 2023

View reviewed changes

Merge branch 'main' into open-zeppelin-audit-cleanup-items

2b308be

michaelkaplan13 merged commit 8822b97 into main Nov 9, 2023

michaelkaplan13 deleted the open-zeppelin-audit-cleanup-items branch November 9, 2023 17:33

OpenZeppelin Audit Cleanup Items #106

OpenZeppelin Audit Cleanup Items #106

Conversation

michaelkaplan13 commented Nov 3, 2023

Why this should be merged

How this works

How this was tested

How is this documented

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this 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

minghinmatthewlam left a comment

Choose a reason for hiding this comment

bernard-avalabs 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