Lines of code
<https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L251&gt;
<https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L565-L581&gt;
<https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L70-L74&gt;

Vulnerability details

Impact

We assume that the admin is honest, however there is still possibility of exploiting asset policy contract to and set price to 0 in oder to buy an asset for free - or even worse - drain user funds by setting the price really high in StandardPolicyERC721::canMatchMakerAsk, thus draining the user balance fron the Pool.

Proof of Concept

Steps to execute:
0. (prerequisite) - change asset matching policy to maliciously set high price and manipulate asset amount, id and type

index 2e24cce..1eb7b98 100644
--- a/contracts/matchingPolicies/StandardPolicyERC721.sol
+++ b/contracts/matchingPolicies/StandardPolicyERC721.sol
@@ -30,7 +30,7 @@ contract StandardPolicyERC721 is IMatchingPolicy {
             (takerBid.amount == 1) &&
             (makerAsk.matchingPolicy == takerBid.matchingPolicy) &&
             (makerAsk.price == takerBid.price),
-            makerAsk.price,
+            1 ether,
             makerAsk.tokenId,
             1,
             AssetType.ERC721
@@ -58,16 +58,12 @@ contract StandardPolicyERC721 is IMatchingPolicy {
             (takerAsk.amount == 1) &&
             (makerBid.matchingPolicy == takerAsk.matchingPolicy) &&
             (makerBid.price == takerAsk.price),
-            makerBid.price,
+            1 ether,
             makerBid.tokenId,
             1,
             AssetType.ERC721
         );

1. create an asset order with really compelling price as a honeypot, and set payment token to the Pool contract address
2. When a victim accepts an offer, take signed order params (frontrun the transaction) and invoke Exchange::execute()
3. Actual price to pay is returned from malicious policy <https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L251&gt; . It’s already set to arbitrarily high amount
4. Exchange::_executeFundsTransfer() is called with the price read from the Policy contract. No additional checks for validity are made, so Exchange::_transferTo <https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Exchange.sol#L581&gt; , and later Pool:transferFrom <https://github.com/code-423n4/2022-11-non-fungible/blob/main/contracts/Pool.sol#L70-L74&gt; is called, which transfers funds to the attacker

Tools Used

VSCode

Recommended Mitigation Steps

Mitigation steps:
The best option is to take the price from buy and sell input, under condition that they do match, as it is not succeptible to this attack. In case that this is not possible, I see two possible options:

1. Introduce grace period before new policy will be active, to at least give users some time to react to the changes
2. Allow user to blacklist certain policy

The text was updated successfully, but these errors were encountered:

All reactions