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

5 stars 0 forks source link

`order.maker` can arbitrarily revoke its approval to cancel its signed orders. #414

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

To create an order, order.maker signs it. Then it is considered valid until it's either cancelled, either filled on chain. But order.maker could use its approvals to PuttyV2 to prevent orders from being valid and effectively cancel them: the tx fillOrder would the revert. This could be used to extract gas from the person filling the order.

Proof of Concept

As a user, if I have submitted an order and sees someone trying to fill it, I can frontrun its transaction to revoke my approvals. In this case he'll pay gas for nothing.

If I use this line for example, the amount of gas burned for nothing could be significant as it's late in the transaction.

Recommended Mitigation Steps

Ways to mitigate this could be:

GalloDaSballo commented 2 years ago

Maker can cancel approvals while order is valid and cause a revert of fillOrder. Order would not be creatable and because of the revert, no assets would be at risk

outdoteth commented 2 years ago

Technically this is true.

It is a gas griefing attack that the maker can do on the taker. The maker will still have to pay gas each time to make this attack successful. So there is a cost - making there be little economic incentive to do so.

It is worth noting that almost all protocols which employ an offchain/onchain settlement mechanism also suffer from this issue.

outdoteth commented 2 years ago

Report: Maker can grief fillOrder by revoking approval, cancelling the original order or using custom baseAssets/assets via mempool frontrunning and cause a revert

HickupHH3 commented 2 years ago

Downgrading to QA because

HickupHH3 commented 2 years ago

part of warden's report: #393