Putty contest - minhquanym's results

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L573

Vulnerability details

Impact

• Function PuttyV2.acceptCounterOffer() is used in case users see better deal and want to cancel their own order before filling the new one. But attacker can abuse this function by front-run calling fillOrder() before it is cancelled in acceptCounterOffer().

• Attackers have motivation to do this (for example: premium). And users who willing to sell only 1 BAYC now may be forced to sell 2 BAYC

Proof of Concept

Scenario

1. Alice posts short call order offchain
2. Bob posts long call order offchain.
3. Alice calls acceptCounterOffer() with Bob’s long call order. But Bob frontruns Alice by filling Alice’s short call order first.
4. Now Alice has 2 short call positions (1 from her own order, 1 from accept Bob’s order) and Bob has 2 long call positions.
5. Alice may be forced to sell more assets than she intended.

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure users cannot cancel() order when the position is minted.

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L367

Vulnerability details

Impact

Takers who fill short-call orders may unable to exercise their long-call position. Attackers can abuse it to take premium from takers for free.

In PuttyV2.fillOrder() function, when taker fill short-call order, order’s maker only has to transfer erc20Assets and erc721Assets in but floor assets is ignored.

After takers filled short-call orders, they will receive long-call position. And because maker didn’t transfer any floor assets in, positionFloorAssetTokenIds[orderHash] is empty, when takers call exercise() the TX will be reverted when trying to transfer floor assets out in this line

Proof of Concept

Scenario

1. Alice posts short call order offchain with floorAssets = [BAYC]
2. Bob calls fillOrder() to fill Alice’s order. In this step, Bob get long call position which should allow him to buy a BAYC from Alice so Bob has to pay Alice premium.
3. Bob calls exercise() to exercise his right but the TX is always reverted.

Tools Used

Manual Review

Recommended Mitigation Steps

In case fillOrder() with short call order, should make sure this order do not use floorAssets

if (!order.isLong && order.isCall) {
    require(order.floorTokens.length == 0);
}

Judge has assessed an item in Issue #253 as Medium risk. The relevant finding follows:

Judge has assessed an item in Issue #253 as Medium risk. The relevant finding follows:

1. Save gas with short-circuit optimization

When evaluating boolean expressions (logical AND and OR) you can stop as soon as you find the first condition which satisfies or negates the expression.

This can be used to optimize gas by reordering conditions in expressions. For example, this expression has 2 conditions

block.timestamp > positionExpirations[longPositionId] || isExercised

First condition reading from storage obviously cost more gas than second condition which simply a boolean variable in stack. We can swap their orders to optimize gas like

isExercised || block.timestamp > positionExpirations[longPositionId]

Affected Codes

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L481

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L327

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L351

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L427