Memory is corrupted when remove asset from token reserve

[code423n4]

Lines of code

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

Vulnerability details

Impact

Impact of memory corruption will appear in case of address update of reserve tokens RDPX/WETH/DPXETH.
I.e. when admin

1. removes old implementation tokens, for example RDPX and DPXETH via removeAssetFromtokenReserves()
2. sets new implementation of RDPX and DPXETH via addAssetTotokenReserves()

After these actions protocol is completely bricked.

Proof of Concept

Corrupted memory layout is hard to illustrate in Solidity, I provide PoC for this.

Please add this function to RdpxV2Core.sol:

  function getReserveAssetSymbol(uint256 index) external view returns (string memory symbol) {
    return reserveAsset[index].tokenSymbol;
  }

Add this test to tests/rdpxV2-core/Admin.t.sol. And run forge test --match-test testCorruptedMemory -vvv

  function testCorruptedMemory() public {
    // remove old implementation
    rdpxV2Core.removeAssetFromtokenReserves("WETH");
    rdpxV2Core.removeAssetFromtokenReserves("RDPX");

    // add new implementation
    rdpxV2Core.addAssetTotokenReserves(
      0x1f9090aaE28b8a3dCeaDf281B0F12828e676c326, // random address
      "WETH"
    );
    rdpxV2Core.addAssetTotokenReserves(
      0x2e08451B79a01Cda253811E45719CeB42640c20d, // random address
      "RDPX"
    );

    // print to console current indexes of tokens
    uint256 a1 = rdpxV2Core.reservesIndex("RDPX");
    uint256 a2 = rdpxV2Core.reservesIndex("WETH");
    uint256 a3 = rdpxV2Core.reservesIndex("DPXETH");
    console.log(a1);
    console.log(a2);
    console.log(a3);
    console.log("-----------------");

    // print to console current order of tokens in `reserveAsset`
    string memory tokenSymbol0 = rdpxV2Core.getReserveAssetSymbol(0);
    string memory tokenSymbol1 = rdpxV2Core.getReserveAssetSymbol(1);
    string memory tokenSymbol2 = rdpxV2Core.getReserveAssetSymbol(2);
    string memory tokenSymbol3 = rdpxV2Core.getReserveAssetSymbol(3);
    console.log(tokenSymbol0);
    console.log(tokenSymbol1);
    console.log(tokenSymbol2);
    console.log(tokenSymbol3);
    console.log("-----------------");

    // print to console current array `reserveTokens`
    string memory b1 = rdpxV2Core.reserveTokens(0);
    string memory b2 = rdpxV2Core.reserveTokens(1);
    string memory b3 = rdpxV2Core.reserveTokens(2);
    console.log(b1);
    console.log(b2);
    console.log(b3);

    // As you can see, `reserveTokens` contains duplicates ["RDPX", "WETH", "RDPX"];

    // Also pointers from `reservesIndex` incorrectly point to `reserveAsset` array
    // For example "WETH" and "DPXETH" point to the same asset WETH
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Current implementation always pops last element of array reserveTokens.

Swap element you want to remove with the last element before pop() like you do with reserveAsset:

    // update the index of reserveAsset with the last element
    reserveAsset[index] = reserveAsset[reserveAsset.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 QA (Quality Assurance)