Skip to content

Latest commit

 

History

History
306 lines (242 loc) · 14.5 KB

075.md

File metadata and controls

306 lines (242 loc) · 14.5 KB

Atomic Butter Bison

High

[H-4] Incorrect deletion logic in auctionFactoryDebita::_deleteAuctionOrder 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 _deleteAuctionOrder function in the auctionFactoryDebita contract contains a bug that results in corruption of the AuctionOrderIndex mapping when deleting the last element in the allActiveAuctionOrders mapping. When deleting the last auction order from the allActiveAuctionOrders mapping, the function incorrectly updates the AuctionOrderIndex mapping by assigning a non-zero index to address(0). This corruption makes the state of the auctionFactoryDebita unreliable and can lead to DoS, loss of funds, or unexpected behavior.

Step-by-Step Breakdown:

  1. Retrieve the index of the auction order to delete uint index = AuctionOrderIndex[_AuctionOrder];

  2. Reset the auction order's index in the mapping AuctionOrderIndex[_AuctionOrder] = 0;

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

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

  5. Update the index mapping for the moved auction order AuctionOrderIndex[allActiveAuctionOrders[index]] = index;

  6. Decrement the active orders count activeOrdersCount--;

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

Root Cause

The issue stems from how the _deleteAuctionOrder function handles the deletion of auction orders, particularly when the auction order to be deleted is the last element in the allActiveAuctionOrders 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
allActiveAuctionOrders = [addr0, addr1, addr2, addr3, addr4]
AuctionOrderIndex[addr0] = 0
AuctionOrderIndex[addr1] = 1
AuctionOrderIndex[addr2] = 2
AuctionOrderIndex[addr3] = 3
AuctionOrderIndex[addr4] = 4

We attempt to delete the last auction order, addr4

Retrieve the index index = AuctionOrderIndex[addr4]; // index = 4

Reset the auction order's index AuctionOrderIndex[addr4] = 0;

Replace the auction order with the last element: Since index = 4 and activeOrdersCount - 1 = 4, the operation becomes: allActiveAuctionOrders[4] = allActiveAuctionOrders[4]; // No change

Remove the last auction order allActiveAuctionOrders[4] = address(0);

Update the index mapping Now, allActiveAuctionOrders[index] is allActiveAuctionOrders[4], which is address(0) after the previous step. Therefore:

AuctionOrderIndex[allActiveAuctionOrders[4]] = index;
// This translates to:
AuctionOrderIndex[address(0)] = 4;

Decrement the active orders count activeOrdersCount = 4;

This corrupts the AuctionOrderIndex mapping by setting an index for address(0). Furthermore, if a new auction is created, this new auction 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 AuctionOrderIndex mapping, and the allActiveAuctionOrders mapping for index 4 will return address(0) instead of the last auction that was created.

Impact

There are multiple issues that stem from this:

  1. AuctionOrderIndex[address(0)] now has a value, which is incorrect. This can lead to unexpected behavior.
  2. The data structures AuctionOrderIndex and allActiveAuctionOrders become inconsistent, making the contract's state unreliable.
  3. Functions that rely on AuctionOrderIndex will retrieve incorrect addresses, leading to logic errors.
  4. The next auction that gets created 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 Auction.t.sol file setup as follows

contract Auction is Test {
//..
//..

+   // Array to hold auction addresses for testing
+   address[] public auctionAddresses;
+   uint[] public veNFTIDs;

    function setUp() public {
-       deal(AERO, signer, 100e18, false);
-       deal(AERO, buyer, 100e18, false);
+      deal(AERO, signer, 1000e18, false);
+      deal(AERO, buyer, 1000e18, false);
        factory = new auctionFactoryDebita();
        ABIERC721Contract = VotingEscrow(veAERO);

//..
//..

        ERC20Mock(AERO).approve(address(ABIERC721Contract), 1000e18);
+       for (uint i = 0; i < 5; i++) {
            uint id = ABIERC721Contract.createLock(100e18, 365 * 4 * 86400);
            ABIERC721Contract.approve(address(factory), id);
            address _auction = factory.createAuction(
                id,
                veAERO,
                AERO,
                100e18,
                10e18,
                86400
            );
+           auctionAddresses.push(_auction);
+       }
        vm.stopPrank();
    }

Now add the following test inside the test file

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

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

        // Delete the last auction order
        address lastAuctionAddress = auctionAddresses[activeOrdersCount - 1];
        DutchAuction_veNFT lastAuction = DutchAuction_veNFT(lastAuctionAddress);

        vm.prank(signer);
        lastAuction.cancelAuction();
        auctionAddresses.pop();

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

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

        console.log("--------------------------------------------------------");
        console.log("----------------- PROVE THE MISMATCH -------------------");
        // Check that the last entry in allActiveAuctionOrders is zero address
        address lastOrder = factory.allActiveAuctionOrders(newActiveOrdersCount);
        assertEq(lastOrder, address(0), "Last order should be address(0) after deletion");
        console.log("Proof that last auction order is set to address(0): ", lastOrder);

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

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

        // Create a new auction order
        vm.startPrank(signer);
        ERC20Mock(AERO).approve(address(ABIERC721Contract), 1000e18);
        uint id = ABIERC721Contract.createLock(100e18, 365 * 4 * 86400);
        ABIERC721Contract.approve(address(factory), id);
        address newAuctionAddress = factory.createAuction(id, veAERO, AERO, 100e18, 10e18, 86400);
        auctionAddresses.push(newAuctionAddress);
        vm.stopPrank();

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

        for (uint i = 0; i < activeOrdersCountNew; i++) {
            address auctionAddress = auctionAddresses[i];
            uint index = factory.AuctionOrderIndex(auctionAddress);
            assertEq(index, i, "AuctionOrderIndex should match index");
            console.log("AuctionOrderIndex before deletion is: ", index);
            address orderAtIndex = factory.allActiveAuctionOrders(i);
            assertEq(orderAtIndex, auctionAddress, "Order at index mismatch");
            console.log("allActiveAuctionOrders before deletion is: ", orderAtIndex);
        }

        address addrLastOrder = factory.allActiveAuctionOrders(activeOrdersCountNew);
        address addrOfLastAuctionCreated = auctionAddresses[4];
        console.log(
            "After the new auction is created, the last auction's address returned by the mappig is still ",
            addrLastOrder,
            "and it should actually point to the address of the last auction order which is this",
            addrOfLastAuctionCreated
        );

        uint zeroAddressIndexAfterNewAuction = factory.AuctionOrderIndex(address(0));
        console.log("address(0) is now mapped to index", zeroAddressIndexAfterNewAuction);
        uint auctionIndexOfNewAuctionAfterDeletion = factory.AuctionOrderIndex(addrOfLastAuctionCreated);
        console.log("Last auction's address is also mapped to index", auctionIndexOfNewAuctionAfterDeletion);
    }

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

Test output

Ran 1 test for test/fork/Auctions/Auction.t.sol:Auction
[PASS] testDeleteLastAuctionOrder() (gas: 1829812)
Logs:
  AuctionOrderIndex before deletion is:  0
  allActiveAuctionOrders before deletion is:  0x104fBc016F4bb334D775a19E8A6510109AC63E00
  AuctionOrderIndex before deletion is:  1
  allActiveAuctionOrders before deletion is:  0x037eDa3aDB1198021A9b2e88C22B464fD38db3f3
  AuctionOrderIndex before deletion is:  2
  allActiveAuctionOrders before deletion is:  0xDDc10602782af652bB913f7bdE1fD82981Db7dd9
  AuctionOrderIndex before deletion is:  3
  allActiveAuctionOrders before deletion is:  0x7FdB3132Ff7D02d8B9e221c61cC895ce9a4bb773
  AuctionOrderIndex before deletion is:  4
  allActiveAuctionOrders before deletion is:  0xfD07C974e33dd1626640bA3a5acF0418FaacCA7a
  --------------------------------------------------------
  --------------------------------------------------------
  AuctionOrderIndex after deletion is:  0
  allActiveAuctionOrders after deletion is:  0x104fBc016F4bb334D775a19E8A6510109AC63E00
  AuctionOrderIndex after deletion is:  1
  allActiveAuctionOrders after deletion is:  0x037eDa3aDB1198021A9b2e88C22B464fD38db3f3
  AuctionOrderIndex after deletion is:  2
  allActiveAuctionOrders after deletion is:  0xDDc10602782af652bB913f7bdE1fD82981Db7dd9
  AuctionOrderIndex after deletion is:  3
  allActiveAuctionOrders after deletion is:  0x7FdB3132Ff7D02d8B9e221c61cC895ce9a4bb773
  --------------------------------------------------------
  ----------------- PROVE THE MISMATCH -------------------
  Proof that last auction order is set to address(0):  0x0000000000000000000000000000000000000000
  Prove that address(0) now has the auction order at index 4
  --------------------------------------------------------
  -----------------  MAPPING CORRUPTED -------------------
  AuctionOrderIndex before deletion is:  0
  allActiveAuctionOrders before deletion is:  0x104fBc016F4bb334D775a19E8A6510109AC63E00
  AuctionOrderIndex before deletion is:  1
  allActiveAuctionOrders before deletion is:  0x037eDa3aDB1198021A9b2e88C22B464fD38db3f3
  AuctionOrderIndex before deletion is:  2
  allActiveAuctionOrders before deletion is:  0xDDc10602782af652bB913f7bdE1fD82981Db7dd9
  AuctionOrderIndex before deletion is:  3
  allActiveAuctionOrders before deletion is:  0x7FdB3132Ff7D02d8B9e221c61cC895ce9a4bb773
  AuctionOrderIndex before deletion is:  4
  allActiveAuctionOrders before deletion is:  0xD76ffbd1eFF76C510C3a509fE22864688aC3A588
  After the new auction is created, the last auction's address returned by the mappig is still  0x0000000000000000000000000000000000000000 and it should actually point to the address of the last auction order which is this 0xD76ffbd1eFF76C510C3a509fE22864688aC3A588
  address(0) is now mapped to index 4
  Last auction's address is also mapped to index 4

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

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

The test clearly shows that the AuctionOrderIndex mapping returns index 4 for both address(0) and the new auction, and the allActiveAuctionOrders mapping returns address(0) for the last auction created instead of the actual auction address.

Mitigation

Modify the auctionFactoryDebita::_deleteAuctionOrder function to correctly handle the deletion of the last element without corrupting the AuctionOrderIndex mapping. The function should perform the swap and mapping update only 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) {
    allActiveAuctionOrders[activeOrdersCount - 1] = address(0);
}