Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: check that order.maker != zero #1188

Merged
merged 4 commits into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 9 additions & 15 deletions packages/marketplace/contracts/exchange/ExchangeCore.sol
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,14 @@ abstract contract ExchangeCore is Initializable, TransferExecutor, ITransferMana
LibOrder.Order memory orderRight,
bytes memory signatureRight
) internal view {
// validate must force order.maker != address(0)
orderValidator.validate(orderLeft, signatureLeft, sender);
orderValidator.validate(orderRight, signatureRight, sender);
if (orderLeft.taker != address(0)) {
if (orderRight.maker != address(0)) require(orderRight.maker == orderLeft.taker, "leftOrder.taker failed");
require(orderRight.maker == orderLeft.taker, "leftOrder.taker failed");
}
if (orderRight.taker != address(0)) {
if (orderLeft.maker != address(0)) require(orderRight.taker == orderLeft.maker, "rightOrder.taker failed");
require(orderRight.taker == orderLeft.maker, "rightOrder.taker failed");
}
}

Expand All @@ -150,8 +151,8 @@ abstract contract ExchangeCore is Initializable, TransferExecutor, ITransferMana
/// @param orderRight the right order of the match
function _matchAndTransfer(
address sender,
LibOrder.Order memory orderLeft,
LibOrder.Order memory orderRight
LibOrder.Order calldata orderLeft,
LibOrder.Order calldata orderRight
) internal {
(LibAsset.AssetType memory makeMatch, LibAsset.AssetType memory takeMatch) = _matchAssets(
orderLeft,
Expand Down Expand Up @@ -188,8 +189,8 @@ abstract contract ExchangeCore is Initializable, TransferExecutor, ITransferMana
/// @return newFill fill result
function _parseOrdersSetFillEmitMatch(
address sender,
LibOrder.Order memory orderLeft,
LibOrder.Order memory orderRight
LibOrder.Order calldata orderLeft,
LibOrder.Order calldata orderRight
)
internal
returns (
Expand All @@ -201,13 +202,6 @@ abstract contract ExchangeCore is Initializable, TransferExecutor, ITransferMana
bytes32 leftOrderKeyHash = LibOrder.hashKey(orderLeft);
bytes32 rightOrderKeyHash = LibOrder.hashKey(orderRight);

if (orderLeft.maker == address(0)) {
orderLeft.maker = sender;
}
if (orderRight.maker == address(0)) {
orderRight.maker = sender;
}

leftOrderData = LibOrderDataGeneric.parse(orderLeft);
rightOrderData = LibOrderDataGeneric.parse(orderRight);

Expand All @@ -231,8 +225,8 @@ abstract contract ExchangeCore is Initializable, TransferExecutor, ITransferMana
/// @return newFill returns change in orders' fills by the match
function _setFillEmitMatch(
address sender,
LibOrder.Order memory orderLeft,
LibOrder.Order memory orderRight,
LibOrder.Order calldata orderLeft,
LibOrder.Order calldata orderRight,
bytes32 leftOrderKeyHash,
bytes32 rightOrderKeyHash,
bool leftMakeFill,
Expand Down
48 changes: 25 additions & 23 deletions packages/marketplace/contracts/exchange/OrderValidator.sol
Original file line number Diff line number Diff line change
Expand Up @@ -44,35 +44,37 @@ contract OrderValidator is IOrderValidator, Initializable, EIP712Upgradeable, Wh
/// @param order order to be validated
/// @param signature signature of order
/// @param sender order sender
function validate(LibOrder.Order memory order, bytes memory signature, address sender) public view {
function validate(LibOrder.Order calldata order, bytes memory signature, address sender) public view {
require(order.maker != address(0), "no maker");

LibOrder.validateOrderTime(order);

address makeToken = abi.decode(order.makeAsset.assetType.data, (address));
verifyWhiteList(makeToken);

if (order.salt == 0) {
if (order.maker != address(0)) {
require(sender == order.maker, "maker is not tx sender");
}
} else {
if (sender != order.maker) {
bytes32 hash = LibOrder.hash(order);
// if maker is contract checking ERC1271 signature
if (order.maker.isContract()) {
require(
IERC1271Upgradeable(order.maker).isValidSignature(_hashTypedDataV4(hash), signature) ==
MAGICVALUE,
"contract order signature verification error"
);
} else {
// if maker is not contract then checking ECDSA signature
if (_hashTypedDataV4(hash).recover(signature) != order.maker) {
revert("order signature verification error");
} else {
require(order.maker != address(0), "no maker");
}
}
}
require(sender == order.maker, "maker is not tx sender");
// No partial fill the order is reusable forever
return;
}

if (sender == order.maker) {
return;
}

bytes32 hash = LibOrder.hash(order);
// if maker is contract checking ERC1271 signature
if (order.maker.isContract()) {
require(
IERC1271Upgradeable(order.maker).isValidSignature(_hashTypedDataV4(hash), signature) == MAGICVALUE,
"contract order signature verification error"
);
return;
}

// if maker is not contract then checking ECDSA signature
if (_hashTypedDataV4(hash).recover(signature) != order.maker) {
revert("order signature verification error");
adjisb marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/marketplace/contracts/exchange/libraries/LibFill.sol
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ library LibFill {
/// @dev 2nd: right order should be fully filled or 3d: both should be fully filled if required values are the same
/// @return the fill result of both orders
function fillOrder(
LibOrder.Order memory leftOrder,
LibOrder.Order memory rightOrder,
LibOrder.Order calldata leftOrder,
LibOrder.Order calldata rightOrder,
uint256 leftOrderFill,
uint256 rightOrderFill,
bool leftIsMakeFill,
Expand Down
4 changes: 2 additions & 2 deletions packages/marketplace/contracts/lib-order/LibOrder.sol
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ library LibOrder {
/// @notice calculate hash key from order
/// @param order object to be hashed
/// @return hash key of order
function hashKey(Order memory order) internal pure returns (bytes32) {
function hashKey(Order calldata order) internal pure returns (bytes32) {
if (order.dataType == DEFAULT_ORDER_TYPE) {
return
keccak256(
Expand Down Expand Up @@ -79,7 +79,7 @@ library LibOrder {
/// @notice calculate hash from order
/// @param order object to be hashed
/// @return hash of order
function hash(Order memory order) internal pure returns (bytes32) {
function hash(Order calldata order) internal pure returns (bytes32) {
return
keccak256(
// solhint-disable-next-line func-named-parameters
Expand Down
43 changes: 43 additions & 0 deletions packages/marketplace/test/exchange/OrderValidator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,46 @@ describe('OrderValidator.sol', function () {
.to.not.be.reverted;
});

it('should validate when salt is non zero and Order maker is sender', async function () {
const {OrderValidatorAsUser, ERC20Contract, ERC721Contract, user1} =
await loadFixture(deployFixtures);
const makerAsset = await AssetERC721(ERC721Contract, 100);
const takerAsset = await AssetERC20(ERC20Contract, 100);
const order = await OrderDefault(
user1,
makerAsset,
ZeroAddress,
takerAsset,
1,
0,
0
);
const signature = await signOrder(order, user1, OrderValidatorAsUser);

await expect(OrderValidatorAsUser.validate(order, signature, user1.address))
.to.not.be.reverted;
});
it('should not validate when maker is address zero', async function () {
const {OrderValidatorAsUser, ERC20Contract, ERC721Contract, user1, user2} =
await loadFixture(deployFixtures);
const makerAsset = await AssetERC721(ERC721Contract, 100);
const takerAsset = await AssetERC20(ERC20Contract, 100);
const order = await OrderDefault(
user1,
makerAsset,
ZeroAddress,
takerAsset,
1,
0,
0
);
order.maker = ZeroAddress;
const signature = await signOrder(order, user2, OrderValidatorAsUser);
await expect(
OrderValidatorAsUser.validate(order, signature, user2.address)
).to.be.revertedWith('no maker');
});

it('should not validate when sender and signature signer is not Order maker', async function () {
const {OrderValidatorAsUser, ERC20Contract, ERC721Contract, user1, user2} =
await loadFixture(deployFixtures);
Expand Down Expand Up @@ -349,4 +389,7 @@ describe('OrderValidator.sol', function () {
)
).to.be.equal(false);
});
// TODO:
// it('should check start / end', async function () {});
// it('should check validate through the whitelist', async function () {});
});
Loading