Smr 2459 invariant testing #89

wcgcyx · 2024-02-27T04:49:39Z

This PR adds invariant tests to the bridge and also enables invariant testing in CI.
Tests are created under test/invariant.
There are 9 invariant tests:

invariant_ERC20TokenBalanced - For every token, total deposited on L1 = total minted on L2.
invariant_IndividualERC20TokenBalanced - For every user and every token, deposited on L1 + minted on L2 stay unchanged.
invariant_IMXBalanced - For IMX, total deposited on L1 = total minted on L2.
invariant_IndividualIMXBalanced - For every user and IMX, deposited on L1 + minted on L2 stay unchanged.
invariant_ETHBalanced - For ETH, total deposited on L1 = total minted on L2.
invariant_IndividualETHBalanced - For every user and ETH, deposited on L1 + minted on L2 stay unchanged.
invariant_NoRemainingWETH - Every WETH is unwrapped automatically so WETH should have no remaining tokens.
invariant_NoRemainingWIMX - Every WIMX is unwrapped automatically so WIMX should have no remaining tokens.
invariant_GasBalanced - The balance of adaptors should match the total amount of gas received.

For each invariant test, forge is configured to generate 15 random calls (in 1 run) distributed uniformly across the following 16 functions and attest invariance after every call:

childBridgeHandler.withdraw.selector;
childBridgeHandler.withdrawTo.selector;
childBridgeHandler.withdrawIMX.selector;
childBridgeHandler.withdrawIMXTo.selector;
childBridgeHandler.withdrawWIMX.selector;
childBridgeHandler.withdrawWIMXTo.selector;
childBridgeHandler.withdrawETH.selector;
childBridgeHandler.withdrawETHTo.selector;
rootBridgeHandler.deposit.selector;
rootBridgeHandler.depositTo.selector;
rootBridgeHandler.depositIMX.selector;
rootBridgeHandler.depositIMXTo.selector;
rootBridgeHandler.depositETH.selector;
rootBridgeHandler.depositETHTo.selector;
rootBridgeHandler.depositWETH.selector;
rootBridgeHandler.depositWETHTo.selector;

Forge is configured to run 256 runs for each test.

platform-sa · 2024-02-27T04:52:44Z

📃CI Report

Compiling 156 files with 0.8.19
Solc 0.8.19 finished in 18.68s
Compiler run �[33msuccessful with warnings:�[0m
�[1;33mWarning (9302)�[0m�[1;37m: Return value of low-level calls not used.�[0m
�[34m |�[0m
�[34m361 |�[0m �[33muser.call{value: amount}("")�[0m;
�[34m |�[0m �[1;33m^^^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m

�[1;33mWarning (9302)�[0m�[1;37m: Return value of low-level calls not used.�[0m
�[34m |�[0m
�[34m174 |�[0m �[33muser.call{value: amount}("")�[0m;
�[34m |�[0m �[1;33m^^^^^^^^^^^^^^^^^^^^^^^^^^^^�[0m

Analysing contracts...
Running tests...
�[2m2024-03-06T04:16:03.924632Z�[0m �[31mERROR�[0m �[2mforge::runner�[0m�[2m:�[0m setUp failed �[3mreason�[0m�[2m=�[0mCould not instantiate forked environment with fork url: http://127.0.0.1:8500 �[3mcontract�[0m�[2m=�[0m0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496

File	% Lines	% Statements	% Branches	% Funcs
src/child/ChildAxelarBridgeAdaptor.sol	100.00% (51/51)	100.00% (68/68)	100.00% (24/24)	100.00% (8/8)
src/child/ChildERC20.sol	100.00% (14/14)	100.00% (15/15)	100.00% (2/2)	100.00% (7/7)
src/child/ChildERC20Bridge.sol	99.23% (129/130)	99.43% (175/176)	98.44% (63/64)	100.00% (21/21)
src/child/WIMX.sol	100.00% (19/19)	100.00% (22/22)	100.00% (8/8)	100.00% (6/6)
src/common/AdaptorRoles.sol	100.00% (6/6)	100.00% (6/6)	100.00% (0/0)	100.00% (6/6)
src/common/BridgeRoles.sol	100.00% (8/8)	100.00% (8/8)	100.00% (0/0)	100.00% (8/8)
src/lib/EIP712MetaTransaction.sol	8.00% (2/25)	9.68% (3/31)	8.33% (1/12)	14.29% (1/7)
src/lib/EIP712Upgradeable.sol	73.33% (11/15)	60.87% (14/23)	0.00% (0/2)	50.00% (2/4)
src/lib/WETH.sol	89.47% (17/19)	86.36% (19/22)	87.50% (7/8)	66.67% (4/6)
src/root/RootAxelarBridgeAdaptor.sol	100.00% (51/51)	100.00% (68/68)	100.00% (24/24)	100.00% (8/8)
src/root/RootERC20Bridge.sol	94.07% (127/135)	95.83% (184/192)	87.93% (51/58)	91.30% (21/23)
src/root/flowrate/FlowRateDetection.sol	100.00% (30/30)	100.00% (33/33)	100.00% (14/14)	100.00% (4/4)
src/root/flowrate/FlowRateWithdrawalQueue.sol	100.00% (46/46)	100.00% (60/60)	78.57% (11/14)	100.00% (7/7)
src/root/flowrate/RootERC20BridgeFlowRate.sol	100.00% (42/42)	97.96% (48/49)	100.00% (10/10)	100.00% (9/9)

For a full HTML report run: forge coverage --report lcov && genhtml --ignore-errors category --branch-coverage --output-dir coverage lcov.info

test/invariant/InvariantBridge.t.sol

ermyas · 2024-02-27T22:40:43Z

test/invariant/InvariantBridge.t.sol

+
+                vm.selectFork(rootId);
+                uint256 balanceL1 = ChildERC20(rootToken).balanceOf(user);
+                address childToken = rootBridge.rootTokenToChildToken(rootToken);


nit: can move outside of internal for-loop?

ermyas · 2024-02-27T22:42:00Z

test/invariant/InvariantBridge.t.sol

+            vm.prank(address(0x234));
+            ChildERC20 rootToken = new ChildERC20();
+            vm.prank(address(0x234));
+            rootToken.initialize(address(123), "Test", "TST", 18);
+            // Mint token to user
+            for (uint256 j = 0; j < NO_OF_USERS; j++) {
+                vm.prank(address(0x234));
+                rootToken.mint(users[j], MAX_AMOUNT);
+            }


nit: suggest startPrank(...) and stopPrank() to avoid doing a separate prank for each call

Yea, updated

ermyas · 2024-02-28T03:55:51Z

test/invariant/InvariantBridge.t.sol

+            assertEq(bridgeBalance, totalSupply);
+            assertEq(bridgeBalance, userBalanceSum);
+        }
+        vm.selectFork(resetId);


are these changes to the resetId fork necessary at the end of each invariant test? I removed all of them and run the tests a few times, and they pass.

ermyas · 2024-02-28T04:30:32Z

test/invariant/child/ChildERC20BridgeHandler.sol

+
+        vm.selectFork(rootId);
+        rootHelper.finaliseWithdrawal(user, previousLen);
+        vm.selectFork(childId);


is this required?

ermyas · 2024-02-28T04:31:27Z

test/invariant/child/ChildERC20BridgeHandler.sol

+            vm.prank(recipient);
+            ChildERC20(rootToken).transfer(user, amount);
+        }
+        vm.selectFork(childId);


given the fork switch right below, is this fork change required?

ermyas · 2024-02-28T23:19:10Z

test/invariant/child/ChildERC20BridgeHandler.sol

+            // Fund difference
+            vm.selectFork(rootId);
+            rootHelper.deposit(user, rootToken, amount - currentBalance, gasAmt);
+            vm.selectFork(childId);


it doesn't look like this is required?

openzeppelin-code · 2024-03-01T04:54:24Z

Smr 2459 invariant testing

Generated at commit: d731c783975bdbbf65a80922bfd60e8b5631dba1

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	1 0 0 7 29 37
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

ermyas · 2024-03-06T02:56:55Z

test/invariant/InvariantBridge.t.sol

+        rootBridge = new RootERC20BridgeFlowRate(address(this));
+        ChildERC20 rootIMXToken = new ChildERC20();
+        rootIMXToken.initialize(address(123), "Immutable X", "IMX", 18);
+        WIMX wETH = new WIMX();


This is slightly confusing. Should this be an instance of WETH contract instead of WIMX, even though they are functionally similar?

Good catch, yes, it is supposed to be WETH - though WIMX is adapted from WETH contract with very minimum change.

ermyas · 2024-03-06T03:10:43Z

test/invariant/InvariantBridge.t.sol

+        }
+        // Create tokens.
+        for (uint256 i = 0; i < NO_OF_TOKENS; i++) {
+            vm.startPrank(address(0x234));


is pranking this address required here?

Yes, my thought is to deploy and mint tokens from a different address other than the testing contract itself so that it is not possible to further mint the token after the setup process in case we add some more logic in the future that may break the setup.

can we add a one line comment on this pls? 🙏🏿

ermyas · 2024-03-06T03:25:44Z

test/invariant/InvariantBridge.t.sol

+        // Deploy contracts on reset chain.
+        vm.selectFork(resetId);
+        vm.startPrank(ADMIN);
+        ChildERC20 resetTokenTemplate = new ChildERC20();
+        resetTokenTemplate.initialize(address(123), "Test", "TST", 18);
+        new MockAdaptor();
+        vm.stopPrank();


I think we can remove the deployment of contracts on the reset chain. Tests should still pass.

Yea, just have this removed and tests passed. I think this was to prevent an issue previously but maybe no longer required.

ermyas · 2024-03-06T03:26:31Z

test/invariant/InvariantBridge.t.sol

+        vm.selectFork(resetId);
+        vm.startPrank(ADMIN);
+        new ChildHelper(payable(childBridge));
+        new RootHelper(ADMIN, payable(rootBridge));
+        new ChildERC20BridgeHandler(childId, rootId, users, rootTokens, address(childHelper), address(rootHelper));
+        new RootERC20BridgeFlowRateHandler(
+            childId, rootId, users, rootTokens, address(childHelper), address(rootHelper)
+        );
+        vm.stopPrank();


I think we can remove the deployment and config of contracts on the reset chain. Tests should still pass.

ermyas · 2024-03-06T03:26:56Z

test/invariant/InvariantBridge.t.sol

+
+            assertEq(balanceL1 + balanceL2, MAX_AMOUNT);
+        }
+        vm.selectFork(resetId);


i think we can remove this

ermyas · 2024-03-06T03:34:37Z

test/invariant/InvariantBridge.t.sol

+    /// forge-config: default.invariant.runs = 256
+    /// forge-config: default.invariant.depth = 15
+    /// forge-config: default.invariant.fail-on-revert = true


Are these local configuration values for the invariant run actually used or is there a global config that overrides them? No matter what value I set, the tests alway run 256 times, with a depth of 15

Yes, they are used, a different value actually gives a different run on my machine.

ermyas · 2024-03-06T03:49:03Z

scripts/localdev/chains.sh

+npx hardhat node --config ./rootchain.config.ts --port 8500 > /dev/null 2>&1 &
+npx hardhat node --config ./childchain.config.ts --port 8501 > /dev/null 2>&1 &
+npx hardhat node --config ./resetchain.config.ts --port 8502 > /dev/null 2>&1 &


I think we can avoid having three chains running, and just use a single chain with different forks from the the same chain?

ermyas · 2024-03-06T03:50:03Z

test/invariant/InvariantBridge.t.sol

+    string public constant CHILD_CHAIN_URL = "http://127.0.0.1:8500";
+    string public constant ROOT_CHAIN_URL = "http://127.0.0.1:8501";
+    // Forge has an issue that fails to reset state at the end of each run.
+    // For example, we found out that if the context stays at child chain at the end of setUp(),
+    // the state on child chain will not be reset or if the context stays at root chain, the state
+    // on the root chain will not be reset, which causes subsequent runs to fail.
+    // We introduced a third chain called reset chain and we make the context to stay on the reset chain
+    // in order to reset state on both child chain and root chain.
+    string public constant RESET_CHAIN_URL = "http://127.0.0.1:8502";


I think we can avoid having three chains running, and just use a single chain with different forks from the the same chain?

Yes, good idea

ermyas · 2024-03-06T04:58:13Z

scripts/localdev/resetchain.config.ts

+      accounts: [],
+    },
+    localhost: {
+      url: "http://127.0.0.1:8502/",


is this required?

wcgcyx added 28 commits February 21, 2024 01:44

Fix crash

b0d64a1

Fix

85a58df

Update InvariantBridge.t.sol

f7baef0

Update

ccf95ab

Update MockAdaptor.sol

de5977f

Update InvariantBridge.t.sol

c2f8e47

Fmt

86a629f

Create resetchain.config.ts

9178788

Update

8081872

Update test.yml

087864f

Update InvariantBridge.t.sol

8255782

Update InvariantBridge.t.sol

da6dcf3

Update InvariantBridge.t.sol

32e49ed

Update InvariantBridge.t.sol

fcfc463

Update InvariantBridge.t.sol

1a36766

Update InvariantBridge.t.sol

f49b8f9

Withdraw/Deposit

5d54819

Update InvariantBridge.t.sol

62e5bc9

Add helper methods

b77a482

Update

dde7380

Update InvariantBridge.t.sol

a5c4fb5

Update RootERC20BridgeFlowRateHandler.sol

1784c91

Simplify testing scenario

5a47e0f

Update

96a9009

Update InvariantBridge.t.sol

cc7342f

Update InvariantBridge.t.sol

2348071

Add tests

83e4f85

Add tests

d30fdc7

ermyas reviewed Feb 27, 2024

View reviewed changes

test/invariant/InvariantBridge.t.sol Outdated Show resolved Hide resolved

ermyas reviewed Feb 27, 2024

View reviewed changes

Update InvariantBridge.t.sol

8a55ab8

ermyas reviewed Feb 28, 2024

View reviewed changes

Cleanup unnecessary code

ea7943f

ermyas reviewed Mar 6, 2024

View reviewed changes

Update InvariantBridge.t.sol

23b96bf

ermyas reviewed Mar 6, 2024

View reviewed changes

wcgcyx added 2 commits March 6, 2024 14:01

Cleanup

7071c46

Cleanup

d731c78

ermyas reviewed Mar 6, 2024

View reviewed changes

scripts/localdev/resetchain.config.ts

accounts: [],

},

localhost: {

url: "http://127.0.0.1:8502/",

Copy link

Collaborator

ermyas Mar 6, 2024

Choose a reason for hiding this comment

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

is this required?

ermyas approved these changes Mar 6, 2024

View reviewed changes

wcgcyx merged commit 562270d into main Mar 6, 2024
5 checks passed

ermyas deleted the SMR-2459-invariant-testing-3 branch April 12, 2024 05:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smr 2459 invariant testing #89

Smr 2459 invariant testing #89

wcgcyx commented Feb 27, 2024 •

edited

Loading

platform-sa commented Feb 27, 2024 •

edited

Loading

ermyas Feb 27, 2024

wcgcyx Feb 28, 2024

ermyas Feb 27, 2024

wcgcyx Feb 28, 2024

ermyas Feb 28, 2024

ermyas Feb 28, 2024

ermyas Feb 28, 2024

ermyas Feb 28, 2024

openzeppelin-code bot commented Mar 1, 2024 •

edited

Loading

ermyas Mar 6, 2024

wcgcyx Mar 6, 2024

wcgcyx Mar 6, 2024

ermyas Mar 6, 2024

wcgcyx Mar 6, 2024

ermyas Mar 6, 2024

ermyas Mar 6, 2024

wcgcyx Mar 6, 2024

ermyas Mar 6, 2024

ermyas Mar 6, 2024

ermyas Mar 6, 2024

wcgcyx Mar 6, 2024

ermyas Mar 6, 2024

ermyas Mar 6, 2024

wcgcyx Mar 6, 2024

ermyas Mar 6, 2024

Smr 2459 invariant testing #89

Smr 2459 invariant testing #89

Conversation

wcgcyx commented Feb 27, 2024 • edited Loading

platform-sa commented Feb 27, 2024 • edited Loading

📃CI Report

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

openzeppelin-code bot commented Mar 1, 2024 • edited Loading

Smr 2459 invariant testing

🚨 Report Summary

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

wcgcyx commented Feb 27, 2024 •

edited

Loading

platform-sa commented Feb 27, 2024 •

edited

Loading

openzeppelin-code bot commented Mar 1, 2024 •

edited

Loading