code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

acceptCounterOffer does not provide floorAssetTokenIds. Assuming the original offer is long call with floorAssetTokenIds, the counter short call must instead have erc721Assets that matches with the long call floorTokens. However there is not checking of that, so the existing info about floorAssetTokenIds will be lost.

Proof of Concept

Tools Used

manual

Recommended Mitigation Steps

if original offer is long call: check:


require(positionFloorAssetTokenIds[uint256(orderHash)].length == erc721Assets.length)
for (uint i; i<erc721Assets.length;) {
     require(positionFloorAssetTokenIds[uint256(orderHash)][i] == erc721Assets[i]);
     unchecked {++i; }
}```

outdoteth commented 2 years ago

There is an incorrect assumption about business logic here.

In the current version of the frontend, it is true that on the UI the counter offer must have the same assets as the original offer. However, in the future it is a reasonable thing to assume that the counter offer may have slightly different underlying assets.

For example;

Alice creates a long call order for 3 floor bayc's Bob creates a counter offer with a short put order for 2 bayc's Alice accepts Bob's counter off

If this is the case, then it does not make sense to have such stringent requirements at the contract level that would prevent this feature.

HickupHH3 commented 2 years ago

agree with sponsor

code-423n4 / 2022-06-putty-findings