Skip to content

Latest commit

 

History

History
312 lines (249 loc) · 14.2 KB

073.md

File metadata and controls

312 lines (249 loc) · 14.2 KB

Atomic Butter Bison

High

[H-3] Incorrect deletion logic in buyOrderFactory::_deleteBuyOrder function leads to mapping corruption

Summary

Note, this issue is present in all the FACTORY contracts. DBOFactory::deleteBorrowOrder, DLOFactory::deleteOrder, auctionFactoryDebita::_deleteAuctionOrder and buyOrderFactory::_deleteBuyOrder

The _deleteBuyOrder function in the buyOrderFactory contract contains a bug that results in corruption of the BuyOrderIndex mapping when deleting the last element in the allActiveBuyOrders mapping. When the last buy order is deleted, the function incorrectly updates the BuyOrderIndex mapping by assigning a non-zero index to address(0). This corruption makes the state of the buyOrderFactory unreliable and can lead to DoS, loss of funds, or unexpected behavior.

Step-by-Step Breakdown:

  1. Retrieve the index of the buy order to delete uint index = BuyOrderIndex[_buyOrder];

  2. Reset the buy order's index in the mapping BuyOrderIndex[_buyOrder] = 0;

  3. Replace the buy order with the last element in the mapping allActiveBuyOrders[index] = allActiveBuyOrders[activeOrdersCount - 1];

  4. Remove the last element from the mapping allActiveBuyOrders[activeOrdersCount - 1] = address(0);

  5. Update the index mapping for the moved buy order BuyOrderIndex[allActiveBuyOrders[index]] = index;

  6. Decrement the active orders count activeOrdersCount--;

The problem arises when the buy order to be deleted is the last element in the mapping.

Root Cause

The issue arises from the way the _deleteBuyOrder function handles the deletion of buy orders, particularly when deleting the last element in the allActiveBuyOrders mapping. The function performs a swap and mapping update even when it's unnecessary, causing the mapping to incorrectly associate address(0) with an index.

Internal pre-conditions

N/A

External pre-conditions

N/A

Attack Path

Example scenario Initial state

activeOrdersCount = 5
allActiveBuyOrders = [addr0, addr1, addr2, addr3, addr4]
BuyOrderIndex[addr0] = 0
BuyOrderIndex[addr1] = 1
BuyOrderIndex[addr2] = 2
BuyOrderIndex[addr3] = 3
BuyOrderIndex[addr4] = 4

Deleting the Last Element (addr4)

_buyOrder = addr4
index = BuyOrderIndex[addr4] = 4
BuyOrderIndex[addr4] = 0

Swap operation allActiveBuyOrders[index] = allActiveBuyOrders[activeOrdersCount - 1];

Since index = 4 and activeOrdersCount - 1 = 4, this results in: allActiveBuyOrders[4] = allActiveBuyOrders[4]; // No change

Nullify the last element allActiveBuyOrders[4] = address(0); // Sets to zero

Update the index mapping BuyOrderIndex[allActiveBuyOrders[index]] = index; At this point, allActiveBuyOrders[index] is address(0), so BuyOrderIndex[address(0)] = 4;

This corrupts the BuyOrderIndex mapping by setting an index for address(0). Furthermore, if a new buyer (Bob) comes in and submits a legitimate buy order, this new order will point out to index 4 as well. The end state is that we will have two different addresses pointing to the same index in the BuyOrderIndex mapping, and the allActiveBuyOrders mapping for index 4 will return address(0) instead of Bob's address

Impact

There are multiple issues that stem from this:

  1. BuyOrderIndex[address(0)] now has a value, which is incorrect. This can lead to unexpected behavior.
  2. The contract's data structures BuyOrderIndex and allActiveBuyOrders become inconsistent, making the contract's state unreliable.
  3. Functions that rely on BuyOrderIndex will retrieve incorrect addresses, leading to logic errors.
  4. The next user's order that gets submitted will point out to the same index that was assigned to address(0). This means that we will have two addresses pointing to the same index. In the case of my example, the index is 4.
  5. The DebitaV3Aggregator contract will work with incorrect data coming from the factory contract.

PoC

Adjust the BuyOrder.t.sol file setup as follows

contract BuyOrderTest is Test {
//..
//..

+   // Array to hold buy order addresses for testing
+   address[] public buyOrderAddresses;

    function setUp() public {
-       deal(AERO, seller, 100e18, false);
-       deal(AERO, buyer, 100e18, false);
+       deal(AERO, seller, 1000e18, false);
+       deal(AERO, buyer, 1000e18, false);

//..
//..

-       // vm.startPrank(buyer);
-       // AEROContract.approve(address(factory), 1000e18);
-       // address _buyOrderAddress = factory.createBuyOrder(
-       //     AERO,
-       //     address(receiptContract),
-       //     100e18,
-       //     7e17
-       // );
-       // buyOrderContract = BuyOrder(_buyOrderAddress);
-       // vm.stopPrank();

+       // Create 5 buy orders
+      vm.startPrank(buyer);
+     AEROContract.approve(address(factory), 1000e18);
+    for (uint i = 0; i < 5; i++) {
+           address _buyOrderAddress = factory.createBuyOrder(
+              AERO,
+              address(receiptContract),
+              100e18,
+               7e17
+          );
+         buyOrderAddresses.push(_buyOrderAddress);
+       }
+       vm.stopPrank();
+   }

Now add the following test inside the test file

    function testDeleteLastBuyOrder() public {
        // Assert initial state
        uint activeOrdersCount = factory.activeOrdersCount();
        assertEq(activeOrdersCount, 5, "Active orders count should be 5");

        // Assert BuyOrderIndex and allActiveBuyOrders before deletion
        for (uint i = 0; i < activeOrdersCount; i++) {
            address buyOrderAddress = buyOrderAddresses[i];
            uint index = factory.BuyOrderIndex(buyOrderAddress);
            assertEq(index, i, "BuyOrderIndex should match index");
            console.log("BuyOrderIndex before deletion is: ", index);
            address orderAtIndex = factory.allActiveBuyOrders(i);
            assertEq(orderAtIndex, buyOrderAddress, "Order at index mismatch");
            console.log("allActiveBuyOrders before deletion is: ", orderAtIndex);
        }

        // Delete the last buy order
        address lastBuyOrderAddress = buyOrderAddresses[activeOrdersCount - 1];
        vm.prank(buyer);
        BuyOrder(lastBuyOrderAddress).deleteBuyOrder();
        buyOrderAddresses.pop();

        // Assert state after deletion
        uint newActiveOrdersCount = factory.activeOrdersCount();
        assertEq(newActiveOrdersCount, 4, "Active orders count should be 4");
        console.log("--------------------------------------------------------");
        console.log("--------------------------------------------------------");

        // Check BuyOrderIndex and allActiveBuyOrders after deletion
        for (uint i = 0; i < newActiveOrdersCount; i++) {
            address buyOrderAddress = buyOrderAddresses[i];
            uint index = factory.BuyOrderIndex(buyOrderAddress);
            assertEq(index, i, "BuyOrderIndex should match index");
            console.log("BuyOrderIndex after deletion is: ", index);
            address orderAtIndex = factory.allActiveBuyOrders(i);
            assertEq(orderAtIndex, buyOrderAddress, "Order at index mismatch after deletion");
            console.log("allActiveBuyOrders after deletion is: ", orderAtIndex);
        }

        console.log("--------------------------------------------------------");
        console.log("----------------- PROVE THE MISMATCH -------------------");
        // Check that the last entry in allActiveBuyOrders is zero address
        address addressLastOrder = factory.allActiveBuyOrders(newActiveOrdersCount);
        assertEq(addressLastOrder, address(0), "Last order should belong to address(0) after deletion");
        console.log("Proof that index of last order returns ", addressLastOrder);

        // Check BuyOrderIndex for address(0)
        uint zeroAddressIndex = factory.BuyOrderIndex(address(0));
        // This should be zero, but due to the bug, it will be 4
        assertEq(zeroAddressIndex, 4, "BuyOrderIndex[address(0)] should be 0");
        console.log("Proof that address(0) is now mapped to buy order at index", zeroAddressIndex);

        console.log("--------------------------------------------------------");
        console.log("-----------------  MAPPING CORRUPTED -------------------");

        //@audit make a new valid order
        vm.startPrank(buyer);
        AEROContract.approve(address(factory), 1000e18);
        address _buyOrderAddress = factory.createBuyOrder(AERO, address(receiptContract), 100e18, 7e17);
        buyOrderAddresses.push(_buyOrderAddress);
        vm.stopPrank();

        uint activeOrdersCountNew = factory.activeOrdersCount();
        assertEq(activeOrdersCountNew, 5, "Active orders count should be 5");

        // Check BuyOrderIndex and allActiveBuyOrders after new order submitted
        for (uint i = 0; i < activeOrdersCountNew; i++) {
            address buyOrderAddress = buyOrderAddresses[i];
            uint index = factory.BuyOrderIndex(buyOrderAddress);
            assertEq(index, i, "BuyOrderIndex should match index");
            console.log("BuyOrderIndex after new order is: ", index);
            address orderAtIndex = factory.allActiveBuyOrders(i);
            assertEq(orderAtIndex, buyOrderAddress, "Order at index mismatch after deletion");
            console.log("allActiveBuyOrders after new order is: ", orderAtIndex);
        }

        address addrLastOrder = factory.allActiveBuyOrders(activeOrdersCountNew);
        address newBuyerAddress = buyOrderAddresses[4];
        console.log(
            "After the new order, the last order's address returned by the mappig is still ",
            addrLastOrder,
            "and it should actually point to the address of the last buyer which is this",
            newBuyerAddress
        );
        uint zeroAddressIndexAfterNewOrder = factory.BuyOrderIndex(address(0));
        console.log("address(0) is now mapped to buy order at index", zeroAddressIndexAfterNewOrder);
        uint buyOrderIndexOfNewBuyerAfterDeletion = factory.BuyOrderIndex(newBuyerAddress);
        console.log(
            "Last buyer's address is now mapped to buy order at index",
            buyOrderIndexOfNewBuyerAfterDeletion,
            "too"
        );

        //@audit we have two addresses pointing to BuyOrder at index[4] and the allActiveBuyOrders[4] returns address(0)
        //instead of the actual user who submitted the last order. Mappings are corrupted
    }

Run forge test --mt testDeleteLastBuyOrder --fork-url https://mainnet.base.org --fork-block-number 21151256 --no-match-path '**Fantom**' -vvv

Test output

Ran 1 test for test/fork/BuyOrders/BuyOrder.t.sol:BuyOrderTest
[PASS] testDeleteLastBuyOrder() (gas: 605802)
Logs:
  BuyOrderIndex before deletion is:  0
  allActiveBuyOrders before deletion is:  0xffD4505B3452Dc22f8473616d50503bA9E1710Ac
  BuyOrderIndex before deletion is:  1
  allActiveBuyOrders before deletion is:  0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81
  BuyOrderIndex before deletion is:  2
  allActiveBuyOrders before deletion is:  0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0
  BuyOrderIndex before deletion is:  3
  allActiveBuyOrders before deletion is:  0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0
  BuyOrderIndex before deletion is:  4
  allActiveBuyOrders before deletion is:  0x6891e60906DEBeA401F670D74d01D117a3bEAD39
  --------------------------------------------------------
  --------------------------------------------------------
  BuyOrderIndex after deletion is:  0
  allActiveBuyOrders after deletion is:  0xffD4505B3452Dc22f8473616d50503bA9E1710Ac
  BuyOrderIndex after deletion is:  1
  allActiveBuyOrders after deletion is:  0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81
  BuyOrderIndex after deletion is:  2
  allActiveBuyOrders after deletion is:  0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0
  BuyOrderIndex after deletion is:  3
  allActiveBuyOrders after deletion is:  0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0
  --------------------------------------------------------
  ----------------- PROVE THE MISMATCH -------------------
  Proof that index of last order returns  0x0000000000000000000000000000000000000000
  Proof that address(0) is now mapped to buy order at index 4
  --------------------------------------------------------
  -----------------  MAPPING CORRUPTED -------------------
  BuyOrderIndex after new order is:  0
  allActiveBuyOrders after new order is:  0xffD4505B3452Dc22f8473616d50503bA9E1710Ac
  BuyOrderIndex after new order is:  1
  allActiveBuyOrders after new order is:  0x8d2C17FAd02B7bb64139109c6533b7C2b9CADb81
  BuyOrderIndex after new order is:  2
  allActiveBuyOrders after new order is:  0x3C8Ca53ee5661D29d3d3C0732689a4b86947EAF0
  BuyOrderIndex after new order is:  3
  allActiveBuyOrders after new order is:  0x76006C4471fb6aDd17728e9c9c8B67d5AF06cDA0
  BuyOrderIndex after new order is:  4
  allActiveBuyOrders after new order is:  0x1fee48ED5BD602834114e19c1a3355b0d20Ea0Df
  After the new order, the last order's address returned by the mappig is still  0x0000000000000000000000000000000000000000 and it should actually point to the address of the last buyer which is this 0x1fee48ED5BD602834114e19c1a3355b0d20Ea0Df
  address(0) is now mapped to buy order at index 4
  Last buyer's address is now mapped to buy order at index 4 too

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 20.70ms (2.08ms CPU time)

Ran 1 test suite in 252.19ms (20.70ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

The test clearly shows that the BuyOrderIndex mapping returns index 4 for both address(0) and the new buy order, and the allActiveBuyOrders mapping returns address(0) for the last buy order instead of the actual buy order address.

Mitigation

Modify the buyOrderFactory::_deleteBuyOrder function to correctly handle the deletion of the last element without corrupting the BuyOrderIndex mapping. The function should only perform the swap and mapping update if the element being deleted is not the last one.

Some idea of custom logic that can be added to the function to handle this edge case.

if (index == activeOrdersCount - 1) {
    allActiveBuyOrders[activeOrdersCount - 1] = address(0);
    BuyOrderIndex[_buyOrder] = 0;
}