RdpxV2Core.removeAssetFromtokenReserves(...) irrecoverably breaks reserve token handling

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L266-L290

Vulnerability details

Vulnerability Details

The reserveTokens storage array is not updated on removing a reserve token by calling RdpxV2Core.removeAssetFromtokenReserves(...), see diff in Recommended Mitigation Steps.
This corrupts the reservesIndex storage mapping as described in the following.

Initial state

Assume the reserve tokens 0x1 - AAA, 0x2 - BBB, 0x3 - CCC and 0x4 - DDD were gracefully added via RdpxV2Core.addAssetTotokenReserves(...) resulting in the state below.

asset index	reserveAsset	reserveTokens	reservesIndex (token symbol -> asset index)
3	...	...	...
4	0x1	AAA	AAA -> 4
5	0x2	BBB	BBB -> 5
6	0x3	CCC	CCC -> 6
7	0x4	DDD	DDD -> 7

State after removing token CCC

After the removal of token 0x3 - CCC via RdpxV2Core.removeAssetFromtokenReserves(...), the reserveTokens array still holds the value CCC instead of DDD since it was never updated. The state is already partially corrupted here.

asset index	reserveAsset	reserveTokens	reservesIndex (token symbol -> asset index)
3	...	...	...
4	0x1	AAA	AAA -> 4
5	0x2	BBB	BBB -> 5
6	0x4	CCC	DDD -> 6

State after removing token BBB

After the removal of token 0x2 - BBB via RdpxV2Core.removeAssetFromtokenReserves(...), the following state issues can be observed:

1. The reserveTokens array still holds the value BBB instead of DDD since it was never updated.
2. Token CCC was "revived" in the reservesIndex mapping and points to asset 0x4 - DDD.
3. The reservesIndex entry for token DDD is now detached and points to a non-existing index, i.e. the reserve cannot be accessed via the reservesIndex mapping anymore.

asset index	reserveAsset	reserveTokens	reservesIndex (token symbol -> asset index)
3	...	...	...
4	0x1	AAA	AAA -> 4
5	0x4	BBB	CCC -> 5
			DDD -> 6

The state is now severely corrupted and it gets worse with each additional reserve token removal.
For comparison, here is how the state table should look like at this point:

asset index	reserveAsset	reserveTokens	reservesIndex (token symbol -> asset index)
3	...	...	...
4	0x1	AAA	AAA -> 4
5	0x4	DDD	DDD -> 5

Impact

As a consequence of the above, the reservesIndex mapping can be corrupted in 2 ways:

1. A mapping entry points to a wrong asset.
2. A mapping entry of a never removed reserve token points to a non-exisiting asset, i.e. the asset was quasi-removed due to the bug. Therefore, accessing, re-adding or removing it will fail, see PoC.

However, the reservesIndex mapping is used at 20 instances troughout the RdpxV2Core contract involving purchase of options, staking, transfers, bonding, settlement, funding, holding the peg and the getReserveTokenInfo(...) method which serves external contracts.
Furthermore, inability to remove a reserve token from the reserveAsset array due to its reservesIndex entry being corrupted/detached leads to DoS of the sync() method in case of token contract failure, which subsequently affects the calling ReLPContract.reLP(...) method.

According to the full product spec which was linked in the README:

rDPX V2 Core:
The backing reserves module will allow using the reserves by the different AMOs. This module should allow different tokens to be used as reserves although the primary backing reserves should remain rDPX and ETH.

Therefore, it is likely that secondary reserve tokens are added and removed during the lifecycle of the protocol which is sufficient to facilitate this vulnerability.
Moreover, even the primary reserves might be removed and re-added in case of migration to a new token contract due to e.g. an uncovered vulnerability. This seems to be an intended use case, otherwise the primary reserve addresses would be immutable instead of being looked up trough a storage array via a mapping which is very costly, e.g. reserveAsset[reservesIndex["RDPX"]].tokenAddress. (It would not be the first time that such emergency actions have to be taken by a DeFi protocol.)

Depending on which reserve tokens are actually affected by the corruption of the reservesIndex mapping, the possible impacts are ranging from DoS concerning interactions with the affected reserves (i.e. slight depeg due to secondary reserves being unavailable through mapping or point to wrong asset) to total failure of the protocol by full DoS of the core functionalities (i.e. severe depeg due to primary reserves being unavailable through mapping or point to wrong asset).

Proof of Concept

The following test case testAddRemoveTokenReserves() proves the discussed vulnerability and is replicating the same states as shown by the tables above.
Just apply the diff below and run the test with
forge test -vv --match-test testAddRemoveTokenReserves:

diff --git a/tests/rdpxV2-core/Unit.t.sol b/tests/rdpxV2-core/Unit.t.sol
index e11c284..c4a5bd9 100644
--- a/tests/rdpxV2-core/Unit.t.sol
+++ b/tests/rdpxV2-core/Unit.t.sol
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity 0.8.19;
 
-import { Test } from "forge-std/Test.sol";
+import { Test, stdError } from "forge-std/Test.sol";
 
 import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
 import { Setup } from "./Setup.t.sol";
@@ -14,6 +14,48 @@ import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVa
 import { IStableSwap } from "contracts/interfaces/IStableSwap.sol";
 
 contract Unit is ERC721Holder, Setup {
+  
+  function testAddRemoveTokenReserves() public {
+    address tokenAddress;
+
+    // add reserve tokens
+    rdpxV2Core.addAssetTotokenReserves(address(0x1), "AAA");
+    rdpxV2Core.addAssetTotokenReserves(address(0x2), "BBB");
+    rdpxV2Core.addAssetTotokenReserves(address(0x3), "CCC");
+    rdpxV2Core.addAssetTotokenReserves(address(0x4), "DDD");
+
+    // check reserve token info
+    (tokenAddress, , ) = rdpxV2Core.getReserveTokenInfo("AAA");
+    assertEq(tokenAddress, address(0x1));
+    (tokenAddress, , ) = rdpxV2Core.getReserveTokenInfo("BBB");
+    assertEq(tokenAddress, address(0x2));
+    (tokenAddress, , ) = rdpxV2Core.getReserveTokenInfo("CCC");
+    assertEq(tokenAddress, address(0x3));
+    (tokenAddress, , ) = rdpxV2Core.getReserveTokenInfo("DDD");
+    assertEq(tokenAddress, address(0x4));
+
+    // remove token CCC and BBB
+    rdpxV2Core.removeAssetFromtokenReserves("CCC");
+    rdpxV2Core.removeAssetFromtokenReserves("BBB");
+
+    // token CCC was "revived" but points to wrong asset
+    vm.expectRevert(abi.encodeWithSelector(RdpxV2Core.RdpxV2CoreError.selector, 19));
+    (tokenAddress, , ) = rdpxV2Core.getReserveTokenInfo("CCC");
+
+    // token DDD is now affected by bug
+    vm.expectRevert(stdError.indexOOBError);
+    (tokenAddress, , ) = rdpxV2Core.getReserveTokenInfo("DDD");
+
+    // can't re-add token DDD again to fix issue
+    // --> reserve functionality of contract broken
+    vm.expectRevert("RdpxV2Core: asset already exists");
+    rdpxV2Core.addAssetTotokenReserves(address(0x4), "DDD");
+
+    // can't remove DDD
+    vm.expectRevert(stdError.indexOOBError);
+    rdpxV2Core.removeAssetFromtokenReserves("DDD");
+  }
+  
   function testBond() public {
     uint256 userRdpxBalance = rdpx.balanceOf(address(this));
     uint256 userwethBalance = weth.balanceOf(address(this));

Tools Used

Manual Review

Recommended Mitigation Steps

In order to mitigate all the discussed issues, the reserveTokens array needs to be updated on reserve token removal:

diff --git a/contracts/core/RdpxV2Core.sol b/contracts/core/RdpxV2Core.sol
index e28a65c..92206eb 100644
--- a/contracts/core/RdpxV2Core.sol
+++ b/contracts/core/RdpxV2Core.sol
@@ -281,6 +281,9 @@ contract RdpxV2Core is
 
     // update the index of reserveAsset with the last element
     reserveAsset[index] = reserveAsset[reserveAsset.length - 1];
+    // update the index of reserveTokens with the last element
+    // index-1 because ZERO was never added to this array
+    reserveTokens[index-1] = reserveTokens[reserveTokens.length - 1];
 
     // remove the last element
     reserveAsset.pop();

Assessed type

Error

[c4-pre-sort]

bytes032 marked the issue as duplicate of #33

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)