EthRouter.change(): msg.value not decremented in loop, cannot process change() call which contains more than one element in the Change[] array

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L273

Vulnerability details

Note that EthRouter.change() can take an array of changes: Change[].

The comment above the function also notes that this function should be able to accept multiple changes.

However, msg.value remains constant throughout each iteration of the loop, which means after the first loop (first change) is done executing, the second loop/change is left with no more msg.value to send.

The offending line of code:
https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/EthRouter.sol#L273

After the first loop, even if the excess is refunded through PrivatePool.sol#L436 the second call will fail because it no longer has the original msg.value balance causing an Out of Funds Revert.

POC

For this test I split the original test to 2 separate changes in the Change[] array. I have avoided minting extra tokens to ensure the weights and Merkle Proof remain the same.

Test file: /test/EthRouter/Change.t.sol

Run test:

forge test --ffi -m test_CallsChangeWithData_2ElementsInChangeArray -vvvv

diff --git a/Change.t.sol b/Change.t_updated.sol
index dbb69ca..d992151 100644
--- a/Change.t.sol
+++ b/Change.t_updated.sol
@@ -137,18 +137,23 @@ contract ChangeTest is Fixture {
         }
     }
 
-    function test_CallsChangeWithData() public {
-        uint256[] memory inputTokenIds = new uint256[](5);
+    function test_CallsChangeWithData_2ElementsInChangeArray() public {
+        uint256[] memory inputTokenIds = new uint256[](2);
         uint256[] memory inputTokenWeights = new uint256[](0);
-        uint256[] memory outputTokenIds = new uint256[](5);
+        uint256[] memory outputTokenIds = new uint256[](2);
         uint256[] memory outputTokenWeights = new uint256[](0);
 
-        for (uint256 i = 0; i < 5; i++) {
+        uint256[] memory inputTokenIds2 = new uint256[](3);
+        uint256[] memory inputTokenWeights2 = new uint256[](0);
+        uint256[] memory outputTokenIds2 = new uint256[](3);
+        uint256[] memory outputTokenWeights2 = new uint256[](0);
+
+        for (uint256 i = 0; i < 2; i++) {
             inputTokenIds[i] = i + 5;
             outputTokenIds[i] = i;
         }
 
-        EthRouter.Change[] memory changes = new EthRouter.Change[](1);
+        EthRouter.Change[] memory changes = new EthRouter.Change[](2);
         changes[0] = EthRouter.Change({
             pool: payable(address(privatePool)),
             nft: address(milady),
@@ -167,8 +172,31 @@ contract ChangeTest is Fixture {
             )
         });
 
+        for (uint256 i = 2; i < 5; i++) {
+            inputTokenIds2[i - 2] = i + 5;
+            outputTokenIds2[i - 2] = i;
+        }
+
+        changes[1] = EthRouter.Change({
+            pool: payable(address(privatePool)),
+            nft: address(milady),
+            inputTokenIds: inputTokenIds2,
+            inputTokenWeights: inputTokenWeights2,
+            inputProof: PrivatePool.MerkleMultiProof(
+                new bytes32[](0),
+                new bool[](0)
+            ),
+            stolenNftProofs: new IStolenNftOracle.Message[](0),
+            outputTokenIds: outputTokenIds2,
+            outputTokenWeights: outputTokenWeights2,
+            outputProof: PrivatePool.MerkleMultiProof(
+                new bytes32[](0),
+                new bool[](0)
+            )
+        });
+
         (uint256 changeFee, ) = privatePool.changeFeeQuote(
-            inputTokenIds.length * 1e18
+            inputTokenIds.length * 1e18 + inputTokenIds2.length * 1e18
         );
 
         // act

Note how the following test fails to execute with an OutOfFund exception.

...
... // first call to change() successful ...
...
    │   ├─ [76892] PrivatePool::change{value: 25000000000000000000}([5, 6], [], ([], []), [], [0, 1], [], ([], [])) 
    │   │   ├─ [5764] StolenNftOracle::validateTokensAreNotStolen(Milady: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], [5, 6], []) 
    │   │   │   └─ ← ()
    │   │   ├─ [419] Factory::protocolFeeRate() [staticcall]
    │   │   │   └─ ← 0
    │   │   ├─ [55] EthRouter::receive{value: 15000000000000000000}() 
    │   │   │   └─ ← ()
...
... // second call to change() failed
...
    │   ├─ [0] PrivatePool::change{value: 25000000000000000000}([7, 8, 9], [], ([], []), [], [2, 3, 4], [], ([], [])) 
    │   │   └─ ← "EvmError: OutOfFund"
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

Test result: FAILED. 0 passed; 1 failed; finished in 3.20ms

Failing tests:
Encountered 1 failing test in test/EthRouter/Change.t_updated.sol:ChangeTest
[FAIL. Reason: EvmError: Revert] test_CallsChangeWithData_2ElementsInChangeArray() (gas: 336274)

Note that both calls to change() send a value of 25000000000000000000, however, the second call no longer has that amount of Eth to send due to the fee from executing the first change() call.

Remediation

msg.value/fees should be tracked and decremented for each iteration of the loop so proper fees are passed upon each call to PrivatePool.change().

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #873

[c4-judge]

GalloDaSballo marked the issue as satisfactory