improve: Make changes after running Slither on SpokePool contracts #229

nicholaspai · 2023-02-04T18:13:37Z

Slither is a Solidity static analysis framework written in Python 3. It runs a
suite of vulnerability detectors, prints visual information about contract details, and provides an API to easily write
custom analyses. Slither enables developers to find vulnerabilities, enhance their code comprehension, and quickly
prototype custom analyses.

Spire-Contracts has been analyzed using Slither@0.9.2 and no major bugs was found. To rerun the analytics, run:

slither contracts/SpokePool.sol
\ --solc-remaps @=node_modules/@
\ --solc-args "--optimize --optimize-runs 1000000"
\ --filter-paths "node_modules"
\ --exclude naming-convention

You can replace SpokePool.sol with the specific contract you want to analyze. For this PR I ran this on the following contracts:

Ovm_SpokePool.sol
Polygon_SpokePool.sol
Optimism_SpokePool.sol
Boba_SpokePool.sol
Ethereum_SpokePool.sol
SpokePool.sol
PolygonTokenBridger

This covered any contracts imported by those contracts. Notably I did not make any changes recommended to HubPool.sol, even though there were some, because we don't have plans to redeploy that soon.

Signed-off-by: nicholaspai npai.nyc@gmail.com

This PR makes SpokePools compatible with OpenZeppelin's upgradeable proxy pattern

UUPS proxies allow the implementation contract to define the conditions under which an upgrade can be called, compared to a Transparent proxy in which only the owner of the proxy can upgrade. This allows the SpokePool upgrades to be triggered by the cross domain admin via the HubPool

into nick/acx-520-add-features-to-spokepool-to-make-future

…use no contracts use delegatecall() anymore

into nick/acx-520-add-features-to-spokepool-to-make-future

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com>

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

into nick/acx-520-add-features-to-spokepool-to-make-future

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

nicholaspai · 2023-02-04T18:14:19Z

contracts/Ovm_SpokePool.sol

@@ -59,6 +59,7 @@ contract Ovm_SpokePool is SpokePool {
        l1Gas = 5_000_000;
        __SpokePool_init(_initialDepositId, _crossDomainAdmin, _hubPool, _wrappedNativeToken, _timerAddress);
        messenger = Lib_PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER;
+        require(_l2Eth != address(0), "Invalid L2 ETH address");


Question: Slither recommends we add these zero address checks. It does add a fixed gas cost to construction but adds some safety. Thoughts?

I'm not sure about the value of these checks. 0 is only one possible invalid address, but there are ~infinitely many other invalid addresses as well. The only way we can really be confident that the address is set correctly is via post-deployment verification.

I'm not firmly against it or anything, it just seems a bit redundant and might give a false sense of security.

i think I agree with your comment and will remove these zero address checks

nicholaspai · 2023-02-04T18:14:46Z

contracts/PolygonTokenBridger.sol

@@ -38,7 +38,7 @@ interface MaticToken {
 *  This ultimately allows create2 to generate deterministic addresses that don't depend on the transaction count of the
 * sender.
 */
-contract PolygonTokenBridger is Lockable {
+contract PolygonTokenBridger is ReentrancyGuard {


Slither had some complaints about Lockable but i figured this was a good time to use OZ's ReentrancyGuard instead

I don't think this contract will be in scope for the audit. Are we okay making this change?

good point, removed

nicholaspai · 2023-02-04T18:15:03Z

contracts/SpokePool.sol

@@ -290,6 +290,11 @@ abstract contract SpokePool is
     * to ensure that a small input range doesn't limit which indices this method is able to reach.
     */
    function emergencyDeleteRootBundle(uint256 rootBundleId) public override onlyAdmin nonReentrant {
+        // Deleting a struct containing a mapping does not delete the mapping in Solidity, therefore the bitmap's


This warning was the most interesting in this PR IMO

Agree - interesting find. It seems OK to suppress the warning:

In the event that a bad bundle makes it to the SpokePool and is subsequently deleted, rootBundle.relayerRefundRoot and rootBundle.slowRelayRoot will be zeroed.

Proof verification is one of the first things to occur in _executeRelayerRefundLeaf() and _executeSlowRelayLeaf().

Any immediate attempt to execute its leaves via executeRelayerRefundRoot() or executeSlowRelayLeaf() should revert on failed proof verification.

This array entry will, at some later time, be overwritten with different data, but the bitmap will indefinitely record that the old deleted leaves remain unclaimed. Any attempt to execute those would rely on collisions and should therefore still fail on proof verification. Wrong & irrelevant, as identified by Matt.

Additionally:

emergencyDeleteRootBundle() is an admin function, so this can't be initiated by any arbitrary caller. To end up here, a specific sequence of unlikely of events needs to occur. It seems highly improbable that a malicious third party could orchestrate that.

I agree with @pxrl. Just to add one point: we will never reuse this array slot (because deleting an element of an array does not actually reduce the array.length I believe, meaning the slot is still reserved), so the mapping will never be read or written to again, by definition, no? So we shouldn't need to worry about this at all right?

Just to add one point: we will never reuse this array slot (because deleting an element of an array does not actually reduce the array.length I believe, meaning the slot is still reserved), so the mapping will never be read or written to again, by definition, no? So we shouldn't need to worry about this at all right?

You're right - I'd understood that the array element was actually removed, but I can see that it's only zeroed. So my 4th point was garbage!

nicholaspai · 2023-02-04T18:15:35Z

contracts/SpokePool.sol

        require(
            getCurrentTime() >= quoteTimestamp - depositQuoteTimeBuffer &&
                getCurrentTime() <= quoteTimestamp + depositQuoteTimeBuffer,
            "invalid quote time"
        );
+
+        // Increment count of deposits so that deposit ID for this spoke pool is unique.


Move the storage variable modification to numberOfDeposits above the external call safeTransferFrom to align with CEI pattern

nicholaspai · 2023-02-04T18:15:51Z

contracts/interfaces/WETH9Interface.sol

@@ -8,5 +8,5 @@ interface WETH9Interface {

    function balanceOf(address guy) external view returns (uint256 wad);

-    function transfer(address guy, uint256 wad) external;
+    function transfer(address guy, uint256 wad) external returns (bool);


This interface was incorrect and inconsistent with how we implement WETH9

pxrl

Good initiative!

pxrl · 2023-02-06T11:15:48Z

contracts/SpokePool.sol

@@ -290,6 +290,11 @@ abstract contract SpokePool is
     * to ensure that a small input range doesn't limit which indices this method is able to reach.
     */
    function emergencyDeleteRootBundle(uint256 rootBundleId) public override onlyAdmin nonReentrant {
+        // Deleting a struct containing a mapping does not delete the mapping in Solidity, therefore the bitmap's


Agree - interesting find. It seems OK to suppress the warning:

In the event that a bad bundle makes it to the SpokePool and is subsequently deleted, rootBundle.relayerRefundRoot and rootBundle.slowRelayRoot will be zeroed.

Proof verification is one of the first things to occur in _executeRelayerRefundLeaf() and _executeSlowRelayLeaf().

Any immediate attempt to execute its leaves via executeRelayerRefundRoot() or executeSlowRelayLeaf() should revert on failed proof verification.

This array entry will, at some later time, be overwritten with different data, but the bitmap will indefinitely record that the old deleted leaves remain unclaimed. Any attempt to execute those would rely on collisions and should therefore still fail on proof verification. Wrong & irrelevant, as identified by Matt.

Additionally:

emergencyDeleteRootBundle() is an admin function, so this can't be initiated by any arbitrary caller. To end up here, a specific sequence of unlikely of events needs to occur. It seems highly improbable that a malicious third party could orchestrate that.

pxrl · 2023-02-06T12:40:26Z

contracts/Ovm_SpokePool.sol

@@ -59,6 +59,7 @@ contract Ovm_SpokePool is SpokePool {
        l1Gas = 5_000_000;
        __SpokePool_init(_initialDepositId, _crossDomainAdmin, _hubPool, _wrappedNativeToken, _timerAddress);
        messenger = Lib_PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER;
+        require(_l2Eth != address(0), "Invalid L2 ETH address");


I'm not sure about the value of these checks. 0 is only one possible invalid address, but there are ~infinitely many other invalid addresses as well. The only way we can really be confident that the address is set correctly is via post-deployment verification.

I'm not firmly against it or anything, it just seems a bit redundant and might give a false sense of security.

pxrl · 2023-02-06T23:26:03Z

README.md

+slither contracts/SpokePool.sol
+\ --solc-remaps @=node_modules/@
+\ --solc-args "--optimize --optimize-runs 1000000"
+\ --filter-paths "node_modules"
+\ --exclude naming-convention


Should we make a shortcut for this via package.json?

hmm i'm having trouble getting this to work from an npm run will look into it further

into npai/slither

mrice32

LGTM!

mrice32 · 2023-02-15T13:38:14Z

contracts/PolygonTokenBridger.sol

@@ -38,7 +38,7 @@ interface MaticToken {
 *  This ultimately allows create2 to generate deterministic addresses that don't depend on the transaction count of the
 * sender.
 */
-contract PolygonTokenBridger is Lockable {
+contract PolygonTokenBridger is ReentrancyGuard {


I don't think this contract will be in scope for the audit. Are we okay making this change?

nicholaspai and others added 25 commits January 4, 2023 17:45

[WIP] feat: Make SpokePool's upgradeable

b9a38f8

This PR makes SpokePools compatible with OpenZeppelin's upgradeable proxy pattern

commit

d409fca

Delete unknown-31337.json

034e801

Update deployments.json

f4185df

Update SpokePool.sol

6db44cd

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

cfdf07e

into nick/acx-520-add-features-to-spokepool-to-make-future

Fix test

54e97c0

Update SpokePool.sol

681a192

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

0ee774e

into nick/acx-520-add-features-to-spokepool-to-make-future

Update SpokePool.Admin.ts

98b272b

No longer require unsafeAllow: delegatecall when deploying proxy beca…

dd41c7b

…use no contracts use delegatecall() anymore

Fix gas tests

77b449a

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

c4c4771

into nick/acx-520-add-features-to-spokepool-to-make-future

Use OZ lockable

7e2b7ed

specify revert reasons

ba9e88f

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

Update contracts/SpokePool.sol

8adde77

Co-authored-by: Paul <108695806+pxrl@users.noreply.github.com>

add 50 32byte slots to base SpokePool

6a9f509

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

add __gap to ovm-spokepool

2932946

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

__gap private

c689fa2

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

ccec16e

into nick/acx-520-add-features-to-spokepool-to-make-future

Update SpokePool.sol

c0bb1b1

Update Ovm_SpokePool.sol

b8fb099

merge

22f5fbe

improve: Make changes after running Slither on SpokePool contracts

cb54ea7

Signed-off-by: nicholaspai <npai.nyc@gmail.com>

nicholaspai commented Feb 4, 2023

View reviewed changes

nicholaspai marked this pull request as ready for review February 4, 2023 18:16

nicholaspai requested review from mrice32 and pxrl February 4, 2023 18:16

Update README.md

5d13900

pxrl reviewed Feb 6, 2023

View reviewed changes

remove zero checks

2ab13f6

Base automatically changed from nick/acx-520-add-features-to-spokepool-to-make-future to master February 10, 2023 20:42

nicholaspai added 2 commits February 13, 2023 10:09

Merge branch 'master' of https://github.com/across-protocol/contracts-v2

c36e9d8

into npai/slither

Update EIP712CrossChainUpgradeable.sol

57855d1

mrice32 approved these changes Feb 15, 2023

View reviewed changes

Update PolygonTokenBridger.sol

bc58c17

nicholaspai merged commit 3ff88e2 into master Feb 16, 2023

nicholaspai deleted the npai/slither branch February 16, 2023 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve: Make changes after running Slither on SpokePool contracts #229

improve: Make changes after running Slither on SpokePool contracts #229

nicholaspai commented Feb 4, 2023

nicholaspai Feb 4, 2023

pxrl Feb 6, 2023

nicholaspai Feb 7, 2023

nicholaspai Feb 4, 2023

mrice32 Feb 15, 2023

nicholaspai Feb 16, 2023

nicholaspai Feb 4, 2023

pxrl Feb 6, 2023 •

edited

Loading

mrice32 Feb 6, 2023

pxrl Feb 6, 2023

nicholaspai Feb 4, 2023

nicholaspai Feb 4, 2023

pxrl left a comment

pxrl Feb 6, 2023 •

edited

Loading

pxrl Feb 6, 2023

pxrl Feb 6, 2023

nicholaspai Feb 8, 2023

mrice32 left a comment

mrice32 Feb 15, 2023

improve: Make changes after running Slither on SpokePool contracts #229

improve: Make changes after running Slither on SpokePool contracts #229

Conversation

nicholaspai commented Feb 4, 2023

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

pxrl Feb 6, 2023 • edited Loading

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

pxrl left a comment

Choose a reason for hiding this comment

pxrl Feb 6, 2023 • edited Loading

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

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxrl Feb 6, 2023 •

edited

Loading

pxrl Feb 6, 2023 •

edited

Loading