Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Commit

Permalink
Patched the exchange
Browse files Browse the repository at this point in the history
  • Loading branch information
@abandeali1
abandeali1 committed Jul 13, 2019
1 parent 201b13c commit ff70c5e
Show file tree
Hide file tree
Showing 7 changed files with 227 additions and 58 deletions.
28 changes: 14 additions & 14 deletions packages/contracts/src/2.0.0/protocol/Exchange/MixinExchangeCore.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,12 @@ contract MixinExchangeCore is
address senderAddress = makerAddress == msg.sender ? address(0) : msg.sender;

// orderEpoch is initialized to 0, so to cancelUpTo we need salt + 1
uint256 newOrderEpoch = targetOrderEpoch + 1;
uint256 newOrderEpoch = targetOrderEpoch + 1;
uint256 oldOrderEpoch = orderEpoch[makerAddress][senderAddress];

// Ensure orderEpoch is monotonically increasing
require(
newOrderEpoch > oldOrderEpoch,
newOrderEpoch > oldOrderEpoch,
"INVALID_NEW_ORDER_EPOCH"
);

Expand Down Expand Up @@ -193,15 +193,15 @@ contract MixinExchangeCore is

// Fetch taker address
address takerAddress = getCurrentContextAddress();

// Assert that the order is fillable by taker
assertFillableOrder(
order,
orderInfo,
takerAddress,
signature
);

// Get amount of takerAsset to fill
uint256 remainingTakerAssetAmount = safeSub(order.takerAssetAmount, orderInfo.orderTakerAssetFilledAmount);
uint256 takerAssetFilledAmount = min256(takerAssetFillAmount, remainingTakerAssetAmount);
Expand All @@ -226,7 +226,7 @@ contract MixinExchangeCore is
orderInfo.orderTakerAssetFilledAmount,
fillResults
);

// Settle order
settleOrder(
order,
Expand Down Expand Up @@ -309,7 +309,7 @@ contract MixinExchangeCore is
order.takerAssetData
);
}

/// @dev Validates context for fillOrder. Succeeds or throws.
/// @param order to be filled.
/// @param orderInfo OrderStatus, orderHash, and amount already filled of order.
Expand All @@ -329,23 +329,23 @@ contract MixinExchangeCore is
orderInfo.orderStatus == uint8(OrderStatus.FILLABLE),
"ORDER_UNFILLABLE"
);

// Validate sender is allowed to fill this order
if (order.senderAddress != address(0)) {
require(
order.senderAddress == msg.sender,
"INVALID_SENDER"
);
}

// Validate taker is allowed to fill this order
if (order.takerAddress != address(0)) {
require(
order.takerAddress == takerAddress,
"INVALID_TAKER"
);
}

// Validate Maker signature (check only if first time seen)
if (orderInfo.orderTakerAssetFilledAmount == 0) {
require(
Expand All @@ -358,7 +358,7 @@ contract MixinExchangeCore is
);
}
}

/// @dev Validates context for fillOrder. Succeeds or throws.
/// @param order to be filled.
/// @param orderInfo OrderStatus, orderHash, and amount already filled of order.
Expand All @@ -381,23 +381,23 @@ contract MixinExchangeCore is
takerAssetFillAmount != 0,
"INVALID_TAKER_AMOUNT"
);

// Make sure taker does not pay more than desired amount
// NOTE: This assertion should never fail, it is here
// as an extra defence against potential bugs.
require(
takerAssetFilledAmount <= takerAssetFillAmount,
"TAKER_OVERPAY"
);

// Make sure order is not overfilled
// NOTE: This assertion should never fail, it is here
// as an extra defence against potential bugs.
require(
safeAdd(orderInfo.orderTakerAssetFilledAmount, takerAssetFilledAmount) <= order.takerAssetAmount,
"ORDER_OVERFILL"
);

// Make sure order is filled at acceptable price.
// The order has an implied price from the makers perspective:
// order price = order.makerAssetAmount / order.takerAssetAmount
Expand All @@ -417,7 +417,7 @@ contract MixinExchangeCore is
// as an extra defence against potential bugs.
require(
safeMul(makerAssetFilledAmount, order.takerAssetAmount)
<=
<=
safeMul(order.makerAssetAmount, takerAssetFilledAmount),
"INVALID_FILL_PRICE"
);
Expand Down
52 changes: 48 additions & 4 deletions packages/contracts/src/2.0.0/protocol/Exchange/MixinSignatureValidator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ contract MixinSignatureValidator is
MTransactions
{
using LibBytes for bytes;

// Mapping of hash => signer => signed
mapping (bytes32 => mapping (address => bool)) public preSigned;

Expand Down Expand Up @@ -197,7 +197,7 @@ contract MixinSignatureValidator is
} else if (signatureType == SignatureType.Validator) {
// Pop last 20 bytes off of signature byte array.
address validatorAddress = signature.popLast20Bytes();

// Ensure signer has approved validator.
if (!allowedValidators[signerAddress][validatorAddress]) {
return false;
Expand Down Expand Up @@ -244,7 +244,17 @@ contract MixinSignatureValidator is
hash,
signature
);
bytes32 magic_salt = bytes32(bytes4(keccak256("isValidWalletSignature(bytes32,address,bytes)")));
assembly {
if iszero(extcodesize(walletAddress)) {
// Revert with `Error("WALLET_ERROR")`
mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
mstore(64, 0x0000000c57414c4c45545f4552524f5200000000000000000000000000000000)
mstore(96, 0)
revert(0, 100)
}

let cdStart := add(calldata, 32)
let success := staticcall(
gas, // forward all gas
Expand All @@ -255,6 +265,15 @@ contract MixinSignatureValidator is
32 // output size is 32 bytes
)

if iszero(eq(returndatasize(), 32)) {
// Revert with `Error("WALLET_ERROR")`
mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
mstore(64, 0x0000000c57414c4c45545f4552524f5200000000000000000000000000000000)
mstore(96, 0)
revert(0, 100)
}

switch success
case 0 {
// Revert with `Error("WALLET_ERROR")`
Expand All @@ -266,7 +285,10 @@ contract MixinSignatureValidator is
}
case 1 {
// Signature is valid if call did not revert and returned true
isValid := mload(cdStart)
isValid := eq(
and(mload(cdStart), 0xffffffff00000000000000000000000000000000000000000000000000000000),
and(magic_salt, 0xffffffff00000000000000000000000000000000000000000000000000000000)
)
}
}
return isValid;
Expand Down Expand Up @@ -294,7 +316,17 @@ contract MixinSignatureValidator is
signerAddress,
signature
);
bytes32 magic_salt = bytes32(bytes4(keccak256("isValidValidatorSignature(address,bytes32,address,bytes)")));
assembly {
if iszero(extcodesize(validatorAddress)) {
// Revert with `Error("VALIDATOR_ERROR")`
mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
mstore(64, 0x0000000f56414c494441544f525f4552524f5200000000000000000000000000)
mstore(96, 0)
revert(0, 100)
}

let cdStart := add(calldata, 32)
let success := staticcall(
gas, // forward all gas
Expand All @@ -305,6 +337,15 @@ contract MixinSignatureValidator is
32 // output size is 32 bytes
)

if iszero(eq(returndatasize(), 32)) {
// Revert with `Error("VALIDATOR_ERROR")`
mstore(0, 0x08c379a000000000000000000000000000000000000000000000000000000000)
mstore(32, 0x0000002000000000000000000000000000000000000000000000000000000000)
mstore(64, 0x0000000f56414c494441544f525f4552524f5200000000000000000000000000)
mstore(96, 0)
revert(0, 100)
}

switch success
case 0 {
// Revert with `Error("VALIDATOR_ERROR")`
Expand All @@ -316,7 +357,10 @@ contract MixinSignatureValidator is
}
case 1 {
// Signature is valid if call did not revert and returned true
isValid := mload(cdStart)
isValid := eq(
and(mload(cdStart), 0xffffffff00000000000000000000000000000000000000000000000000000000),
and(magic_salt, 0xffffffff00000000000000000000000000000000000000000000000000000000)
)
}
}
return isValid;
Expand Down
8 changes: 7 additions & 1 deletion packages/contracts/src/2.0.0/test/ReentrantERC20Token/ReentrantERC20Token.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,4 +185,10 @@ contract ReentrantERC20Token is
// Transfer will return true if function failed for any other reason
return true;
}
}

// This is used in the Order Wallet tests
function ()
external
payable
{}
}
2 changes: 1 addition & 1 deletion packages/contracts/src/2.0.0/test/TestExchangeInternals/TestExchangeInternals.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ contract TestExchangeInternals is
{
return isRoundingErrorCeil(numerator, denominator, target);
}

/// @dev Updates state with results of a fill order.
/// @param order that was filled.
/// @param takerAddress Address of taker who filled the order.
Expand Down
61 changes: 59 additions & 2 deletions packages/contracts/test/exchange/wrapper.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { BlockchainLifecycle } from '@0xproject/dev-utils';
import { assetDataUtils, orderHashUtils } from '@0xproject/order-utils';
import { RevertReason, SignedOrder } from '@0xproject/types';
import { Order, RevertReason, SignedOrder } from '@0xproject/types';
import { BigNumber } from '@0xproject/utils';
import { Web3Wrapper } from '@0xproject/web3-wrapper';
import * as chai from 'chai';
Expand All @@ -13,7 +13,7 @@ import { ERC721ProxyContract } from '../../generated_contract_wrappers/erc721_pr
import { ExchangeContract } from '../../generated_contract_wrappers/exchange';
import { ReentrantERC20TokenContract } from '../../generated_contract_wrappers/reentrant_erc20_token';
import { artifacts } from '../utils/artifacts';
import { expectTransactionFailedAsync } from '../utils/assertions';
import { expectTransactionFailedAsync, expectTransactionFailedWithoutReasonAsync } from '../utils/assertions';
import { getLatestBlockTimestampAsync, increaseTimeAndMineBlockAsync } from '../utils/block_timestamp';
import { chaiSetup } from '../utils/chai_setup';
import { constants } from '../utils/constants';
Expand Down Expand Up @@ -1447,6 +1447,63 @@ describe('Exchange wrappers', () => {
);
expect(expiredOrderInfo.orderStatus).to.be.equal(expectedExpiredOrderStatus);
});

it('should throw if wallet signature is used with an eoa', async () => {
const currentTimestamp = (new BigNumber(await getLatestBlockTimestampAsync())).plus(new BigNumber(100000));

const order: Order = {
senderAddress: constants.NULL_ADDRESS,
makerAddress: makerAddress,
takerAddress: constants.NULL_ADDRESS,
makerFee: new BigNumber(0),
takerFee: new BigNumber(0),
makerAssetAmount: new BigNumber(4000000000),
takerAssetAmount: new BigNumber(100),
makerAssetData: assetDataUtils.encodeERC20AssetData(defaultMakerAssetAddress),
takerAssetData: assetDataUtils.encodeERC20AssetData(defaultTakerAssetAddress),
salt: currentTimestamp,
exchangeAddress: exchange.address,
feeRecipientAddress: constants.NULL_ADDRESS,
expirationTimeSeconds: currentTimestamp,
};

const signedOrder: SignedOrder = {
...order,
signature: '0x04',
};
return expectTransactionFailedWithoutReasonAsync(
exchangeWrapper.fillOrderAsync(signedOrder, takerAddress),
);
})

it('should throw if wallet signature is used with a contract that returns nothing', async () => {
const currentTimestamp = (new BigNumber(await getLatestBlockTimestampAsync())).plus(new BigNumber(100000));

const order: Order = {
senderAddress: constants.NULL_ADDRESS,
makerAddress: reentrantErc20Token.address,
takerAddress: constants.NULL_ADDRESS,
makerFee: new BigNumber(0),
takerFee: new BigNumber(0),
makerAssetAmount: new BigNumber(4000000000),
takerAssetAmount: new BigNumber(100),
makerAssetData: assetDataUtils.encodeERC20AssetData(defaultMakerAssetAddress),
takerAssetData: assetDataUtils.encodeERC20AssetData(defaultTakerAssetAddress),
salt: currentTimestamp,
exchangeAddress: exchange.address,
feeRecipientAddress: constants.NULL_ADDRESS,
expirationTimeSeconds: currentTimestamp,
};

const signedOrder: SignedOrder = {
...order,
signature: '0x04',
};

return expectTransactionFailedWithoutReasonAsync(
exchangeWrapper.fillOrderAsync(signedOrder, takerAddress),
);
})
});
});
}); // tslint:disable-line:max-file-line-count
8 changes: 8 additions & 0 deletions packages/migrations/src/2.0.0/migration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,14 @@ export const runV2MigrationsAsync = async (provider: Provider, artifactsDir: str
from: owner,
}),
);

// Register Asset Proxies to the MAP
await web3Wrapper.awaitTransactionSuccessAsync(
await multiAssetProxy.registerAssetProxy.sendTransactionAsync(erc20proxy.address, { from: owner }),
);
await web3Wrapper.awaitTransactionSuccessAsync(
await multiAssetProxy.registerAssetProxy.sendTransactionAsync(erc721proxy.address, { from: owner }),
);
// Transfer MAP ownership
await web3Wrapper.awaitTransactionSuccessAsync(
await multiAssetProxy.transferOwnership.sendTransactionAsync(assetProxyOwner.address, {
Expand Down

0 comments on commit ff70c5e

Please sign in to comment.