Skip to content

Commit

Permalink
fix(signing): improve checks submitted orders
Browse files Browse the repository at this point in the history
  • Loading branch information
@bitbeckers
bitbeckers committed May 24, 2024
1 parent 52ea602 commit 6ada61b
Show file tree
Hide file tree
Showing 6 changed files with 347 additions and 11 deletions.
62 changes: 51 additions & 11 deletions contracts/src/marketplace/executionStrategies/StrategyHypercertFractionOffer.sol
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.17;

// Interface
import {IHypercertToken} from "@hypercerts/protocol/interfaces/IHypercertToken.sol";

// Libraries
import {OrderStructs} from "../libraries/OrderStructs.sol";

Expand All @@ -26,6 +23,14 @@ import {
// Base strategy contracts
import {BaseStrategy, IStrategy} from "./BaseStrategy.sol";

interface IHypercert1155Token {
function unitsOf(uint256 tokenId) external view returns (uint256);

function ownerOf(uint256 tokenId) external view returns (address);

function isApprovedForAll(address account, address operator) external view returns (bool);
}

/**
* @title StrategyHypercertFractionOffer
* @notice This contract offers a single execution strategy for users to bid on
Expand Down Expand Up @@ -69,11 +74,22 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
returns (uint256 price, uint256[] memory itemIds, uint256[] memory amounts, bool isNonceInvalidated)
{
itemIds = makerAsk.itemIds;

// A collection order can only be executable for 1 itemId but the actual quantity to fill can vary
if (makerAsk.amounts.length != 1 || itemIds.length != 1) {
revert LengthsInvalid();
}

//TODO Apply to other HC strats
if (
makerAsk.signer != IHypercert1155Token(makerAsk.collection).ownerOf(itemIds[0])
&& !IHypercert1155Token(makerAsk.collection).isApprovedForAll(
IHypercert1155Token(makerAsk.collection).ownerOf(itemIds[0]), makerAsk.signer
)
) {
revert OrderInvalid();
}

// The amount to fill must be 1 because fractions are NFTs
if (makerAsk.amounts[0] != 1) {
revert AmountInvalid();
Expand Down Expand Up @@ -101,12 +117,12 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
// If the unitAmount is lower than the specified minUnitAmount to sell
if (unitAmount < minUnitAmount) {
// We expect to sale to be executed only if the units held are equal to the minUnitsToKeep
if (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount != minUnitsToKeep) {
if (IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount != minUnitsToKeep) {
revert OrderInvalid();
}
} else {
// Don't allow the sale to let the units held get below the minUnitsToKeep
if ((IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep) {
if ((IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep) {
revert OrderInvalid();
}
}
Expand All @@ -117,7 +133,7 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
}

// Don't allow the sale to let the units held get below the minUnitsToKeep
if ((IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep) {
if ((IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep) {
revert OrderInvalid();
}
}
Expand All @@ -130,7 +146,8 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
price = unitAmount * pricePerUnit;
// If the amount to fill is equal to the amount of units in the hypercert, we transfer the fraction.
// Otherwise, we do not invalidate the nonce because it is a partial fill.
isNonceInvalidated = (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) == minUnitsToKeep;
isNonceInvalidated =
(IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) == minUnitsToKeep;
}

/**
Expand Down Expand Up @@ -165,6 +182,16 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
revert AmountInvalid();
}

//TODO Apply to other HC strats
if (
makerAsk.signer != IHypercert1155Token(makerAsk.collection).ownerOf(itemIds[0])
&& !IHypercert1155Token(makerAsk.collection).isApprovedForAll(
IHypercert1155Token(makerAsk.collection).ownerOf(itemIds[0]), makerAsk.signer
)
) {
revert OrderInvalid();
}

//units, pricePerUnit, proof[]
(uint256 unitAmount, uint256 pricePerUnit, bytes32[] memory proof) =
abi.decode(takerBid.additionalParameters, (uint256, uint256, bytes32[]));
Expand All @@ -179,15 +206,15 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
if (
minUnitAmount > maxUnitAmount || unitAmount == 0 || unitAmount > maxUnitAmount
|| pricePerUnit < makerAsk.price || makerAsk.price == 0
|| (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep
|| (IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep
) {
revert OrderInvalid();
}
} else {
if (
minUnitAmount > maxUnitAmount || unitAmount == 0 || unitAmount < minUnitAmount
|| unitAmount > maxUnitAmount || pricePerUnit < makerAsk.price || makerAsk.price == 0
|| (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep
|| (IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) < minUnitsToKeep
) {
revert OrderInvalid();
}
Expand All @@ -201,7 +228,8 @@ contract StrategyHypercertFractionOffer is BaseStrategy {

// If the amount to fill is equal to the amount of units in the hypercert, we transfer the fraction.
// Otherwise, we do not invalidate the nonce because it is a partial fill.
isNonceInvalidated = (IHypercertToken(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) == minUnitsToKeep;
isNonceInvalidated =
(IHypercert1155Token(makerAsk.collection).unitsOf(itemIds[0]) - unitAmount) == minUnitsToKeep;

bytes32 node = keccak256(abi.encodePacked(takerBid.recipient));

Expand Down Expand Up @@ -238,7 +266,19 @@ contract StrategyHypercertFractionOffer is BaseStrategy {
if (
makerAsk.amounts.length != 1 || makerAsk.itemIds.length != 1 || minUnitAmount > maxUnitAmount
|| makerAsk.price == 0 || maxUnitAmount == 0
|| IHypercertToken(makerAsk.collection).unitsOf(makerAsk.itemIds[0]) <= minUnitsToKeep
|| IHypercert1155Token(makerAsk.collection).unitsOf(makerAsk.itemIds[0]) <= minUnitsToKeep
) {
return (isValid, OrderInvalid.selector);
}

// Because the split call is made by the marketplace which has the makers approval, we need to check whether the
// signer is the owner of the hypercert
// This is a side-effect of the procotol design as theres no `operator` declared in the split function
if (
makerAsk.signer != IHypercert1155Token(makerAsk.collection).ownerOf(makerAsk.itemIds[0])
&& !IHypercert1155Token(makerAsk.collection).isApprovedForAll(
IHypercert1155Token(makerAsk.collection).ownerOf(makerAsk.itemIds[0]), makerAsk.signer
)
) {
return (isValid, OrderInvalid.selector);
}
Expand Down
75 changes: 75 additions & 0 deletions contracts/test/foundry/marketplace/LooksRareProtocol.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ import {
CURRENCY_NOT_ALLOWED,
MAKER_ORDER_INVALID_STANDARD_SALE
} from "@hypercerts/marketplace/constants/ValidationCodeConstants.sol";
// Strategies
import {StrategyHypercertFractionOffer} from
"@hypercerts/marketplace/executionStrategies/StrategyHypercertFractionOffer.sol";

// Other mocks and utils
import {MockERC20} from "../../mock/MockERC20.sol";
Expand All @@ -36,13 +39,29 @@ contract LooksRareProtocolTest is ProtocolBase {

// Mock files
MockERC20 private mockERC20;
StrategyHypercertFractionOffer public strategyHypercertFractionOffer;
bytes4 public selectorNoProof = StrategyHypercertFractionOffer.executeHypercertFractionStrategyWithTakerBid.selector;
bytes4 public selectorWithProofAllowlist =
StrategyHypercertFractionOffer.executeHypercertFractionStrategyWithTakerBidWithAllowlist.selector;

uint256 private constant minUnitAmount = 1;
uint256 private constant maxUnitAmount = 100;
uint256 private constant minUnitsToKeep = 5000;
bool private constant sellLeftover = false;

function setUp() public {
_setUp();
_setUpNewStrategies();
vm.prank(_owner);
mockERC20 = new MockERC20();
}

function _setUpNewStrategies() private asPrankedUser(_owner) {
strategyHypercertFractionOffer = new StrategyHypercertFractionOffer();
_addStrategy(address(strategyHypercertFractionOffer), selectorNoProof, false);
_addStrategy(address(strategyHypercertFractionOffer), selectorWithProofAllowlist, false);
}

function testCannotTradeIfInvalidAmounts() public {
_setUpUsers();

Expand Down Expand Up @@ -275,4 +294,60 @@ contract LooksRareProtocolTest is ProtocolBase {
takerBids, makerAsks, signatures, merkleTrees, isAtomic
);
}

function testCannotExecuteOnERC721TokensNotOwnedByMsgSender() public {
(address anon, uint256 anonPK) = makeAddrAndKey("anon");

_setUpUser(anon);
_setUpUsers();
vm.prank(_owner);
looksRareProtocol.updateCurrencyStatus(address(mockERC20), true);

uint256 itemId = 420;
mockERC721.mint(makerUser, itemId);

OrderStructs.Maker memory makerAsk = _createSingleItemMakerOrder({
quoteType: QuoteType.Ask,
globalNonce: 0,
subsetNonce: 0,
strategyId: STANDARD_SALE_FOR_FIXED_PRICE_STRATEGY,
collectionType: CollectionType.ERC721,
orderNonce: 0,
collection: address(mockERC721),
currency: ETH,
signer: anon,
price: 1 ether,
itemId: itemId
});

OrderStructs.Taker memory takerBid = OrderStructs.Taker(takerUser, abi.encode());

makerAsk.currency = address(mockERC20);

mockERC20.mint(takerUser, 10 ether);
vm.prank(takerUser);
mockERC20.approve(address(looksRareProtocol), 10 ether);

mockERC20.mint(anon, 10 ether);
vm.prank(anon);
mockERC20.approve(address(looksRareProtocol), 10 ether);

// Sign order
bytes memory signature = _signMakerOrder(makerAsk, anonPK);

uint256 makerNftBalance = mockERC721.balanceOf(makerUser);
uint256 makerErc20Balance = mockERC20.balanceOf(makerUser);

vm.prank(anon);
vm.expectRevert();
looksRareProtocol.executeTakerBid(takerBid, makerAsk, signature, _EMPTY_MERKLE_TREE);

assertEq(makerNftBalance, 1);

assertEq(mockERC721.balanceOf(anon), 0);
assertEq(mockERC721.balanceOf(makerUser), 1);
assertEq(mockERC721.balanceOf(takerUser), 0);

assertEq(mockERC20.balanceOf(makerUser), makerErc20Balance);
}
}
8 changes: 8 additions & 0 deletions contracts/test/foundry/marketplace/ProtocolBase.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,17 @@ contract ProtocolBase is MockOrderGenerator, ILooksRareProtocol {
MockHypercertMinterUUPS public mockHypercertMinterUUPS;
HypercertMinter public mockHypercertMinter;

// enum TransferRestrictions {
// AllowAll,
// DisallowAll,
// FromCreatorOnly
// }

IHypercertToken.TransferRestrictions public constant FROM_CREATOR_ONLY =
IHypercertToken.TransferRestrictions.FromCreatorOnly;

IHypercertToken.TransferRestrictions public constant ALLOW_ALL = IHypercertToken.TransferRestrictions.AllowAll;

ProtocolFeeRecipient public protocolFeeRecipient;
LooksRareProtocol public looksRareProtocol;
TransferManager public transferManager;
Expand Down
72 changes: 72 additions & 0 deletions contracts/test/foundry/marketplace/executionStrategies/HypercertCollectionOffers.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import {CollectionType} from "@hypercerts/marketplace/enums/CollectionType.sol";
import {QuoteType} from "@hypercerts/marketplace/enums/QuoteType.sol";

contract HypercertCollectionOffersTest is ProtocolBase {
error ERC1155SafeTransferFromFail();

StrategyHypercertCollectionOffer public strategyHypercertCollectionOffer;
bytes4 public selectorNoProof =
strategyHypercertCollectionOffer.executeHypercertCollectionStrategyWithTakerAsk.selector;
Expand Down Expand Up @@ -691,6 +693,76 @@ contract HypercertCollectionOffersTest is ProtocolBase {
assertEq(errorSelector, QuoteTypeInvalid.selector);
}

function testCannotExecuteOnHypercertCollectionOfferNotOwnedBySigner() public {
(address anon, uint256 anonPK) = makeAddrAndKey("anon");

// All users and anon have given approval to the marketplace on hypercerts protocol
_setUpUsers();
_setUpUser(anon);
vm.prank(_owner);
looksRareProtocol.updateCurrencyStatus(address(weth), true);

uint256 _units = 10_000;
// Mint asset
vm.prank(takerUser);
mockHypercertMinter.mintClaim(takerUser, _units, "https://example.com", ALLOW_ALL);

uint256[] memory itemIds = new uint256[](1);
itemIds[0] = (1 << 128) + 1;

uint256[] memory amounts = new uint256[](1);
amounts[0] = 1;

// Anon creates a maker bid order for a fraction owned by takerUser on behalf of makerUser
OrderStructs.Maker memory makerBid = _createSingleItemMakerOrder({
quoteType: QuoteType.Bid,
globalNonce: 0,
subsetNonce: 0,
strategyId: 1,
collectionType: CollectionType.Hypercert,
orderNonce: 0,
collection: address(mockHypercertMinter),
currency: address(weth),
signer: makerUser,
price: price,
itemId: 0 // not used
});

makerBid.additionalParameters = abi.encode(10_000);

// The strategy will interpret the order as invalid
(bool isValid,) = strategyHypercertCollectionOffer.isMakerOrderValid(makerBid, selectorNoProof);
assertTrue(isValid);

// Prepare the taker ask where the anon will execute the order on behalf of the takerUser
OrderStructs.Taker memory takerAsk = OrderStructs.Taker(anon, abi.encode(firstHypercertFractionId, 10_000));

// Maker has signed the order
bytes memory signature = _signMakerOrder(makerBid, makerUserPK);

uint256 takerBeforeBalance = weth.balanceOf(takerUser);
uint256 anonBeforeBalance = weth.balanceOf(anon);
uint256 makerBeforeBalance = weth.balanceOf(makerUser);

assertEq(mockHypercertMinter.ownerOf(itemIds[0]), takerUser);

// Anon will get rejected because they're not allowed to execute the order on behalf of the taker
vm.prank(anon);
vm.expectRevert(ERC1155SafeTransferFromFail.selector);
looksRareProtocol.executeTakerAsk(takerAsk, makerBid, signature, _EMPTY_MERKLE_TREE);

// Anon pranks user to execute the order on behalf of the maker
// This means a user want funds received from sale to go to a different account, which should be allowed
vm.prank(takerUser);
looksRareProtocol.executeTakerAsk(takerAsk, makerBid, signature, _EMPTY_MERKLE_TREE);

// Nothing changed
assertEq(mockHypercertMinter.ownerOf(itemIds[0]), makerUser);
assertEq(weth.balanceOf(takerUser), takerBeforeBalance);
assertTrue(weth.balanceOf(anon) > anonBeforeBalance);
assertTrue(weth.balanceOf(makerUser) < makerBeforeBalance);
}

function _assertOrderIsValid(OrderStructs.Maker memory makerBid, bytes4 strategySelector) private {
(bool orderIsValid, bytes4 errorSelector) =
strategyHypercertCollectionOffer.isMakerOrderValid(makerBid, strategySelector);
Expand Down
Loading

0 comments on commit 6ada61b

Please sign in to comment.