Summary

Currently the peer allows trades with order.taker.wallet of any address. While this doesn't enable any of the Peer owner's tokens to be taken, it does mean that the Peer's rules can be utilized by other addresses, thereby draining the maxTakerAmount when the true owner hasn't carried out trades.

Changes

• Add tradeWallet address to the peer, which is set by the owner
• Add a require that the order.taker.wallet == tradeWallet in the provideOrder function.

The addition of tradeWallet means that the Peer's owner can store their tokens in a different wallet (e.g. a hardware wallet) from the wallet they used to deploy the contract (e.g. a hot wallet), whilst still allowing them to ensure no one uses their peer.

Ensure that the order that a Peer returns is actually fillable
do a balance, approval, and authorization prior to returning peerLocator and amount

Sample pseudocode:

  * @notice Provides check on Peer to ensure meets reqs for complete trade
  * @dev notice this is just checking peer not checking if peerfrontend is ok
  * @param _peerAddress address
  * @param _takerAmount uint256
  * @param _takerToken address
  * @return uint256
  */
  function checkPeerValidity(
  	address _peerAddress, 
  	uint256 _takerAmount, 
  	address _takerToken
  ) 
  external view returns (uint256 allowedAmount) {
  
      address tradeWalletAddress = IPeer(_peerAddress).tradeWallet()
      bool authd = Swap(_swapAddress).delegateApprovals(tradeWalletAddress, _peerAddress)
      uint256 currentAllowance = IERC20(_takerToken).allowance(tradeWalletAddress, _swapAddress)
  	if (authd) {
  		if (currentAllowance > _takerAmount) {
  			return _takerAmount;
  		} else {
  			return currentAllowance;
  		}
  	}
  	return 0;
  }

In the current setup for Indexer and Index, to fetch a set of intents to trade, the caller can specify the number of intents to trade n, and they are returned the first n intents in the linked list. As the system grows and the linked list gets long, it may be that systems want/need to paginate through the intents in groups. For this reason it would be helpful to allow the starting point to be specified when fetching intents, instead of implicitly starting at the head of the list every time.

Changes:
The following 2 functions each need a second argument address startPoint that specifies where the start of the traversal should be.
Indexer.fetchIntents
Index.getLocators

Currently the pricing logic on Delegate will drop decimals. While these amounts are small, we should be intentional about when to round values up or down, and the result should never be zero.

Found that not all contract return a bool on approve either similar to the transferFrom.

While most of these are not likely to become staking tokens used in the Indexer, wanted to call out these tokens are not suitable stakingTokens

PLR
BOXX
DAT
CDT
LRC
DNA
MTL
LNK
BNN
PAY
GOLD
ADX
USDT
IBTC
PAN

Description

Separate the approvals to differentiate approvals for the different parties in a Swap order.

signerApprovals allows a Party to have someone else be their primary signer to submit orders such that a signature is not required.

example: The authorized signer may be a externally owned wallet while the signerWallet may actually be a smart contract.

senderApprovals allows a Party to have someone else be their primary sender of orders.

Changes

Addition

mapping (address => mapping (address => uint256)) public signerApprovals;
mapping (address => mapping (address => uint256)) public senderApprovals;

authorizeSigner(address _authorizedAddress, _expiry)
authorizeSender(address _authorizedAddress, _expiry)
revokeSigner(address _authorizedAddress)
revokeSender(address _authorizedAddress)

Removal:

mapping (address => mapping (address => uint256)) public delegateApprovals;

Tests

Ensure that approvals stay clear cut

• Check that a signerApprovals does not impact a sender
• Check that a senderApprovals does not impact a signer
  Check on different combinations of the parties being authorized
• Ensure the msg.sender is the party that can authorize or revoke their approval

1. rename blacklist -> tokenBlackList
2. bool public contractPaused = false; doesn't need to be declared, it does this by default.
3. In natspec in setLocatorWhitelist, what does this line mean and should it be removed?
   • @dev Clear the whitelist with a null address (0x0)"
4. rename in setIntent, amount -> stakingAmount
5. switch the ordering so the emit Stake is the last call in setIntent
6. getStakedAmount, because it is an external view function. I'd like to remove this check. If it fails below, that's fine.
   require(indexes[signerToken][senderToken] != Index(0),
   "INDEX_DOES_NOT_EXIST");
   Reference: https://forum.openzeppelin.com/t/require-in-view-pure-functions-dont-revert-on-public-networks/1211
7. createIndex, add comment
   @dev if an Index already exists, it will return the address of the Index and not emit a CreateIndex event

Currently, almost none of the return values of function calls are documented.

Make the events indexed for the Delegate

  event SetRule(
    address indexed senderToken,
    address indexed signerToken,
    uint256 maxSenderAmount,
    uint256 priceCoef,
    uint256 priceExp
  );

  event UnsetRule(
    address indexed senderToken,
    address indexed signerToken
  );

Consequences:

There will be additional gas costs as indexed params require this.
the formula for emitting an event is:
k = 375, a is 8, and b is 375
k + unindexedBytes * a + indexedTopics * b
so will be adding 750 gas to each event emittance for the 2 indexed params.

Changes

With the additional support of REVERT reasons in Etherscan, may want to make reason for friendly.
Still will keep them < bytes32 but can offer more guidance and more intuitive verbiage.

Currently, only the Owner of a Peer has the right to manage its rules. This is a proposal to enable the Owner of a Peer to give other parties the right to manage its rules.

i.e. if a Manager is to act on behalf of an owner in order to add a rule and set an intent through a Peer. Peer needs the ability to grant such control.

• the contract should have an owner
• the contract should have a list of parties that can set and unset an intent; a whitelist.
• owner is always apart of whitelist

This is a dependency for #144

Current Behavior:

  event SetRule(
    address senderToken,
    address signerToken,
    uint256 maxSenderAmount,
    uint256 priceCoef,
    uint256 priceExp
  );

  event UnsetRule(
    address senderToken,
    address signerToken
  );

Proposed Behavior

  event SetRule(
    address ruleOwner,
    address senderToken,
    address signerToken,
    uint256 maxSenderAmount,
    uint256 priceCoef,
    uint256 priceExp
  );

  event UnsetRule(
    address ruleOwner,
    address senderToken,
    address signerToken
  );

Reasons

Allows for developers/dapps to build delegate trackers and make it easier for users to track when something has been set or unset

Currently an indexer will happily let a user set intent for an arbitrary delegate contract. This means that a consumer must accept that all delegates are untrusted.

If however we create a delegate factory that the indexer trusts, then a consumer can transitively trust indexed delegates. Would like to host discussion here and explore developing a trusted delegate factory.

BLUF: Saving about 128 bytes with this change per Order. I also examined Peer and Indexer but did not find areas for optimization.

Solidity works in 32 bytes slots and so if we can fit fixed params within a single slot, re-arranged the params to do just that.

Current

  struct Signature {
    address signer;       // 20 byte
    uint8 v;              // 1 byte 
    bytes32 r;            // 32 bytes
    bytes32 s;            // 32 bytes
    bytes1 version;       // bytes 1
  }
  4 - 32 bytes

  struct Party {
    address wallet;       // 20 byte
    address token;        // 20 byte
    uint256 param;        // 32 bytes
    bytes4 kind;          // 4 bytes
  }

  4 - 32 bytes 

  struct Order {
    uint256 nonce;        // 32 bytes
    uint256 expiry;       // 32 bytes
    Party maker;          // 4 - 32 bytes
    Party taker;          // 4 - 32 bytes
    Party affiliate;      // 4 - 32 bytes
    Signature signature;  // 4 - 32 bytes
  }
  18 - 32 bytes

Proposed

  struct Signature {
    address signer;       //20 bytes
    bytes1 version;       //1 bytes
    uint8 v;              //1 byte
    bytes32 r;            //32 bytes
    bytes32 s;            //32 byetes
  }
  3 - 32 bytes

  struct Party {
    bytes4 kind;          // 4 bytes
    address wallet;       // 20 bytes
    address token;        // 20 bytes
    uint256 param;        // 32 bytes
    
  }
  3 - 32 bytes

  struct Order {
    uint256 nonce;        // 32 bytes
    uint256 expiry;       // 32 bytes
    Party maker;          // 3 - 32 bytes
    Party taker;          // 3 - 32 bytes
    Party affiliate;      // 3 - 32 bytes
    Signature signature;  // 3 - 32 bytes
  }
14 - 32 bytes

We use the smallest unit for every token in all scenarios. This should be documented along with any usage of a token amount to reiterate this.

For example, WETH has 18 decimal places. While traders may think in terms of 1 WETH, the actual value within the contracts would be 1000000000000000000.

As the repo has grown, the different folders have become less structured, and the line between helper vs protocol has become more blurred.

This is just a reminder we should consider the stucture of the repo while we're tidying it up pre-launch.

We currently do not have test coverage reporting instrumented for any package. The solidity-coverage project has been good when it works, but we have not had success configuring it yet.

Would like to add solidity-coverage or an alternative so we can ensure proper test coverage.

When a market reaches a certain size, getIntents and findPosition will stop working for state-changing transactions. We should determine the limits in different scenarios and decide whether to hardcode a cap or just publish the limit.

Contracts like AirSwapToken explicitly prevent transfers and transferFrom of zero amounts and will revert.

Thus any places we transfer tokens in delegate or indexer should bypass trying to transfer 0 amount as that will leave indices non-removable right now.

Currently calling getIntents on an indexer returns all types of intents. The locator value of an intent indicates with the final byte its type, whether a delegate contract, instant peer, or arbitrary URL. Being able to filter getIntents by type would be useful, particularly in onchain integrations that are only able to interface with delegate contracts.

Summary

This issue is to propose the renaming of the maker and taker variable names to signer and sender. A detailed description of the reasons for this proposal is given below.

Motivation

• Maker describes the party who is setting prices on the protocol
• Taker describes the party who accepts prices, and completes a trade
• When it comes to the swap contract, it should not matter who provided liquidity and who decided to take it, what matters is that both parties involved agreed to the swap.
• In every simple decentralised trade, party A signs an order to show that they agree with it, and party B sends it to the smart contract to show that they too agree with it.
• It doesn't matter whether A was the maker or the taker in the order, the signature from A and the transaction from B show that both parties wanted the trade.
• The swap contract always calls the party who signed the order the maker and the sender of the transaction the taker - despite the fact that the party who signed the order could have been the taker of liquidity.

Example

• One example where the terms maker and taker become muddled is with on-chain delegate contracts.
  Deploying a delegate on-chain and giving it permission to trade for you provides liquidity to the market, and the contract is an on-chain market maker.
• However when a trade is submitted to the Swap contract from the delegate contract, the delegate is set as the taker of the order.
• This is due to the fact that the delegate is the party that sends the order, and the Swap contract requires the taker to be the party that sends the order.
• The party that interacts with the delegate contract and takes liquidity is the party who signs the order in this situation. This means that they are set as the maker of the order so that Swap can process the signature correctly.

Conclusion

• It feels like terms have become overloaded, no longer meaning what they were initially intended to mean, which makes the code harder to understand and reason about.
• I propose with the names sender and signer it would abstract away from liquidity and trade initialisation, and instead focus the contracts back onto the approvals of the trade and the security of the smart contracts.

Consequences

• This issue is to show that maker is not synonymous with signer, and the same with taker and sender.
• This is not to suggest that the names maker and taker are removed from Airswap entirely, but just from the Swap part of the protocol - contract, interacting libraries etc.
• Trades will still have makers and takers of liquidity, which will remain relevant throughout other contracts, libraries and on the website.
• Swap.swap(Order order) sample obj

  "nonce": "100",
  "expiry": "1566941284",
  "signer": {
    "wallet": "0x655...",
    "token": "0x27054b13b1b798b345b591a4d22e6562d47ea75a" (AST),
    "param": "10000",
    "kind": "0x277f8169"
  },
  "sender": {
    "wallet": "0xdea...",
    "token": "0xc778417e063141139fce010982780140aa0cd5ab (WETH)",
    "param": "100000000",
    "kind": "0x277f8169"
  },
  "affiliate": {
    "wallet": "0x0000000000000000000000000000000000000000",
    "token": "0x0000000000000000000000000000000000000000",
    "param": "0",
    "kind": "0x277f8169"
  },
    "signature": {
      "signatory": "0xdea...",
      "version": "0x45",
      "r": "0x589...",
      "s": "0x730...",
      "v": "28"
    }
}

As Index could be a standalone component, would like to put its features in the contract natspec

Current contract Natspec:

/**
  * @title Index: A List of Locators
  */

Proposed

/**
  * @title Index: A List of Locators
  * @notice The Locators are sorted in reverse order based on the score
  * meaning that the head has the largest score and tail is the smallest
 * @dev A mapping is used to mimic a circular linked list structure
* where every mapping value contains a pointer to the next
  */

When using the Indexer, the score and identifier are also important information to have when fetching locators.

• score: The user needs to know how much they must to stake to rank on the Index.
• identifier: The user needs to have the identifier of at least the final entry for pagination.

They should be returned in the same call to represent the same state.

A route may be to return e.g. (bytes32[], uint256[], address[]) from the getLocators call for locators, scores, and identifiers respectively. Alternatively, if only the last entry is useful for pagination, (bytes32[], uint256[], address).

Check the changes in the new version and upgrade the repo to the most recent Solidity version 0.5.12.

Currently the functions in Types.sol (our types library) are marked as external. This means we have to deploy and link the Types library for any contract that uses it. Due to the fact that the Types library is used rarely, and the number of uses of the 2 functions are once and twice respectively, it may make sense to make the functions as internal. This would mean we don't have to deploy Types.sol at all, and the contracts that utilise it can pull in what they need for deployment.

Currently, for example, with existing locator scores of 2, 1, 1 a new locator with score 1 will be placed at the end of the list ([2, 1, 1, 1]). We should consider adding the new locator at the beginning e.g. [2, 1, 1, 1].

This would encourage fresher locators in the higher rankings, and given that makers will need to observe the Indexer for changes anyway, would not require special or additional work from makers.

Purpose

As AirSwap wants to expand the token types supported, creating a more generalized solution for supporting more token types.

The below document discusses why the current setup does not work for the current tokens AirSwap would like to support and certain short term solutions.
Alt to Token Registry idea if just trying to support non-standard ERC20 and ERC721

• Using multiple if-then conditionals for different kinds all within the Swap contract in this case, pull in multiple different interfaces
• Hardcode the specific USDT address and OMG address to have custom behavior for ERC20 contract
• With Cryptokitty, easy fix would be to use transferFrom instead of safeTransferFrom, and have the UI ensure that the _to receive is always going to be a valid receiver for all other ERC721 tokens
• In addition, transferFrom in IERC721 does not return a bool it only returns success or fails
• Modify open-zeppelin, IERC20.sol interface so that the transferFrom function no longer returns bool values

New token types:
ERC1155, ERC721, ERC20, and custom tokens like USDT and KittyCore

Requirements

• Need a way for Swap.swap to work irrespective of what irregular tokens are used if they need to be supported with front end applications.
• Inserting the TransferHandlerRegistry into the Swap contract

Interface ITransferHandler.sol

function transferTokens(address _from, address _to, uint256 _param, uint256 amount, uint256 id, address token,) external returns bool;

Sample ERC20TransferHandler

    import IERC20.sol
    import "./ITransferHandler.sol";
import SafeERC20.sol
    
    contract ERC20TransferHandler is ITransferHandler {
    using SafeERC20 for IERC20;
    		function transferTokens(address from, address to, uint256 amount, uint256 id, address *token*) return (bool){
                                require(id == 0, "ID_INVALID");
    				require(IERC20(token).transferFrom(from, to, amount));
    				return true;
    		}
    }

Sample CryptoKittyTransferHandler

    /**
     * @title IKittyCoreTokenTransfer
     * @dev transferFrom function from KittyCore
     */
    interface IKittyCoreTokenTransfer {
        function transferFrom(address from, address to, uint256 amount, uint256 id) external;
    }
    
    contract KittyCoreTransferHandler is ITransferHandler {
    
        function transferTokens(address from, address_to, uint256 amount, uint256 id, address token) return (bool) {
            require(amount == 0, "AMOUNT_INVALID");
            require(IKittyCoreTokenTransfer(token).transferFrom(from, to, id));
            return true;
        }
    }

Sample ERC721TransferHandler

    import "./ITransferHandler.sol";
    import "openzeppelin-solidity/contracts/token/ERC20/IERC721.sol";
    
    contract ERC721TransferHandler is ITransferHandler {
        function transferTokens(address from, address to, uint256 amount, uint256 id, address token) return (bool) {
            require(amount == 0, "AMOUNT_INVALID");
            IERC721(_token).safeTransferFrom(from, to, id);
            return true;
        }
    }

Contract TransferHandlerRegistry.sol

• purpose of this contract is the manage and store the bytes4 → ITransferHandler
• Will need to only allow certain whitelisted parties to update the registry and make it controlled
• Fluidity can be the primary owner
• An owner can only add handlers to the registry, nothing can be removed

Functions

• addTransferHandler(bytes4 kind, ITransferHandler transferHandler)
  • emits event AddHandler(bytes4 kind, address asset)

Contract Swap.sol changes

• Swap will contain a call to the TransferHandlerRegistry which has a mapping that goes from bytes4 → ITransferHandler()
• Swap removes the hard coded interface identifiers
• another contract will handle this mapping TransferHandlerRegistry which will need to be injected into the constructor of swap

    require(from != to, "INVALID_SELF_TRANSFER");
    ITransferHandler transferHandler = registry.transferHandlers(kind);
    require(address(transferHandler) != address(0), "UNKNOWN_TRANSFER_HANDLER");
    // delegatecall required to pass msg.sender as Swap contract to handle the
    // token transfer in the calling contract
    (bool success, bytes memory data) = address(transferHandler).
        delegatecall(abi.encodeWithSelector(
          transferHandler.transferTokens.selector,
          from,
          to,
          amount,
          id,
          token
    ));
    require(success && abi.decode(data, (bool)), "TRANSFER_FAILED");

Tests

• Full test path coverage

• Ensure that no one else can modify or update the TransferHandlerRegistry (or allow it to be added to by an owner)

• Using SafeERC20 allows us to not have a two distinct handlers with ERC20 contracts anymore, they both will pass through to IERC20.

Currently if you try to setIntent over an existing intent, it will fail with ENTRY_ALREADY_EXISTS. However, there is no way to look this up ahead of time.

Don: Propose that we change the visibility of Index.entries to public so this can be checked before a user tries to update their intent.

Currently all tests rely on the tests before them executing correctly. Tests should be independent of one another. Snapshots allow all tests to work from the same start state.

Introduction

• The design of the wrapper contract is purposefully such that makers trade WETH, whereas takers may prefer to trade ETH.
• This means that the wrapper was designed such that only the sender could trade ETH (as the sender is traditionally the order taker).
• However, with the introduction of on-chain makers (aka delegate contracts), the order taker will now sometimes be the order signer.
• This needs to be updated for wrapper to work with delegates.
• Furthermore, for the authorizations of addresses to send or sign needs to be considered in this more complex situation.

Signer and sender updates to Wrapper

• The wrapper needs to be updated to allow either party to be the party sending or receiving ETH; currently this must be the sender.
• This can be done by pulling certain information out of the order into variables at the start of Wrapper.swap. Then having generic code that can execute using these variables.
• It needs to be decided whether ETH can be traded for WETH - if so, the ETH address should be provided as 0xFFFFFFF or 0x0. I think this would make the order clearer.

Authorizations

• Currently a require statement exists in Wrapper.swap:

    require(_order.sender.wallet == msg.sender, "MSG_SENDER_MUST_BE_ORDER_SENDER");

• This would prevent the delegate trading through the wrapper, as the sender.wallet is not the delegate
• A require could be added Swap.isAuthorizedSender(sender.wallet, msg.sender), or to check sender.wallet == Delegate(msg.sender).tradeWallet

• Structs in libraries are merely copied into the contracts they are used within, and do not reference the library itself after compilation
• Types is a library that is used as a helper to Swap - it stores constants and functions that are used to implement EIP712 for swap, and help with the security of the contract.
• It feels like Rule is out of place in this library - it is not used within Swap, and is not used in the implementation of EIP712 for Swap.
• Rule is only used within the (I)Delegate contract, and so belongs there, not in the Types library.

Moving from #226 as the proposed changes evolved away from the original topic there.

Description

In regards to: https://www.notion.so/fluidity/Indexer-Audit-ef2f668a578f4fefa050442933683bac
Related to issue: #170

Add Ownable functionality and Pausable functionality to the Indexer.

Existing Functionality

contract Indexer is IIndexer, Ownable {

Proposed Functionality

contract Indexer is IIndexer, Ownable, Pausable {

We should also add additional functionality that will allow refunding of staked amounts if we've needed to pause due to a security vulnerability.

function refundStake(address) onlyOwner {

This will only be possible if the contract were paused.

Reason

As we're in the on going process of development. Until the contracts are audited and we have 100% confidence in the code we should be playing it safe.

Once audited and 100% confident we can relinquish the ownership so that it's entirely decentralized.

Considerations

During development and before audited the Indexer will be centralized and controlled.

Motivation

Given that Peer.sol partially implements the Peer API we think that naming it Delegate.sol is an opportunity to define such a partially implemented Peer. Indeed, this contract behaves as a delegate for another peer in the system.

Proposed Renaming:

IPeer.sol -> IDelegate.sol

Peer.sol -> Delegate.sol

IPeerFactory.sol -> IDelegateFactory.sol

PeerFactory.sol -> DelegateFactory.sol

Contract DelegateFactory:

• Implements ILocatorWhitelist
• Its purpose is to create new Delegates

Currently if a user wants to update their amount staked on a market, they have to call unsetIntent and then setIntent, which requires them signing and sending 2 transactions. Considering the indexer will revolve around only being able to return the first n staked locators, it should be expected that people might want to update their stake to ensure they remain competitive. For this reason I propose a function updateStakeAmount(signerToken, senderToken, newAmount that updates the user's staked amount. This function need not be complex - it could merely call unsetIntent and setIntent internally, but it stops the user from having to sign 2 transactions.

Currently, the authorization feature, including both signer and sender authorization, takes an expiry value after which the authorization is invalid.

What we see, and will likely see often, is that in practice, these authorizations are set with essentially infinite expiry. This is a proposal to consider removing expiry in favor of a boolean value.

In this discussion we could consider alternate forms of controlling authorizations, for example, specifying a maximum number of swaps that can be performed for a given authorization.

Index is more of independent data structure versus an application specific contract.

Putting down a placeholder to think more carefully if it can or should be shifted to be a library.

Description

In the current setup of the Delegate/Indexer/Index, a user cannot have multiple intents to trade on the same token pair. This is a situation that should be allowed on the contracts - take the following situation for example:

• At the rate of 200 DAI per WETH, Bob is willing to buy 5 WETH.
• At the rate of 80 DAI per WETH, Bob is willing to buy a lot more WETH - maybe 20

To set up such trades automatically on a Delegate contracts Bob would need multiple Rules on the same token pair, which isn't possible. Bob could instead deploy a second Delegate for his second rule (which is non-ideal but possible). However to actually stake his intent to trade on an Index (which is a token-pair-specific indexer), each user can only have one stake, due to the implementation using a mapping of user address.

This thereby makes the above situation impossible.

Changes

• Allow multiple intents to trade to be set by one user on an Index. This would mean the mapping should be indexed by something different - for example hash(userAddress, nonce)
• Ideally also allow multiple rules to be set on the same Delegate for the same token pair. This could involve a mapping with nonces again, or better storing an array of Rules on each pairing.

IIndexer.sol

Currently there are no indexed params in the Indexer, the below is my proposed change. I have not added/removed any parameters, that can also be up for discussion. I've added indexes to allow somone to query based on user wallets as well as signerToken/senderToken (ie tokenPair).

  event CreateIndex(
    address  signerToken,
    address senderToken
  );

  event Stake(
    address indexed wallet,
    address indexed signerToken,
    address indexed senderToken,
    uint256 amount
  );

  event Unstake(
    address indexed wallet,
    address indexed signerToken,
    address indexed senderToken,
    uint256 amount
  );

  event AddToBlacklist(
    address token
  );

  event RemoveFromBlacklist(
    address token
  );

Consequences:

There will be additional gas costs as indexed params require this.
the formula for emitting an event is:
k = 375, a is 8, and b is 375
k + unindexedBytes * a + indexedTopics * b

so staked and unstake will add 1125 to each event as there are now 3 indexed topics.

Problem

The DelegateFactory can be injected to create a Delegate where the swap address is any address.

function createDelegate(address _swapContract, address _delegateContractOwner) external returns (address delegateContractAddress) {

This gives the potential for somebody to create a Delegate that doesn't actually use AirSwap's Swap contract.

If the DelegateFactory contains a list of deployedAddresses that is referenced by
setIntent when creating an intent

    // Ensure the locator is whitelisted, if relevant
    if (locatorWhitelist != address(0)) {
      require(ILocatorWhitelist(locatorWhitelist).has(_locator),
      "LOCATOR_NOT_WHITELISTED");
    }

this doesn't really mean much if the created address can be compromised, see below.
this was added into Peer.js ('Peer' is in reference to Delegate)

    it('Corrupt Peer with specialized swap', async () => {
      let wrapper = await Wrapper.new(swapAddress, tokenWETH.address);

      //provide peer with wrapper and not the swap address; this could be any address that swaps
      let corruptPeer = await Peer.new(wrapper.address, aliceAddress)
      await corruptPeer.setRule(
        tokenDAI.address,
        tokenDAI.address,
        10,
        1,
        0
      )

      //alice authorizes the peer
      await swapContract.authorize(corruptPeer.address, 1577836800, { from: aliceAddress });

      // Note: Consumer is the order maker, Peer is the order taker.
      const order = await orders.getOrder({
        maker: {
          wallet: bobAddress,
          token: tokenDAI.address,
          param: 1,
        },
        taker: {
          wallet: corruptPeer.address,
          token: tokenDAI.address,
          param: 1,
        },
      })

      // Bob submits to corrupt peer
      await passes(
        corruptPeer.provideOrder(order, { from: bobAddress }),
      )
    })

Proposed Solution

I am suggesting the DelegateFactory to be provided a swap address on creation. This way DelegateFactory will create Delegates for a particular swap address and give merit to its deployedAddresses.

DelegateFactory will need to have a constructor with the swap address that sets the swapContract state variable:

constructor(address _swapContract) {
    swapContract = _swapContract;
}

The factory method also needs to be updated:

function createDelegate(address _swapContract, address _delegateContractOwner)

to become

function createDelegate(address _delegateContractOwner)

Given that there may be many Swap contracts, whether for different systems, or through incremental versions, two peers will need to agree on which contract they intend to settle trades.

The value of version would be a Swap contract address.

Names are based off the renaming proposed here:

Delegate Renamings

Note: All names used in this issue will be according to the namings set out in issue 137. However this renaming is not necessary for the concept of the Manager to be understood and discussed. If 137 is not approved, this Manager could be implemented using the old naming conventions

Motivation:

• The Manager concept is based on the idea to provide users with a way to access all of their created Delegates and simplify the setting of rules and intents.
• Currently a user creates a new Delegate using the createDelegate() method from the DelegateFactory
• Since this Delegate is brand new it has no rules and no intents set on Indexer. A user would first need to send a transaction to setRule() on the Delegate and then subsequently send another transaction to setIntent() on the Indexer. Multiple transactions are necessary to setup the Delegate
• Since setting a rule and setting an intent usually go hand in hand it's possible to package both of these in a singular transaction.

Contract DelegateManager:

• The user interfaces with this contract

• The manager acts on behalf of a user, keeping track of all the Delegates constructed for a particular owner using a mapping(address ⇒ address[])

• The manager facilitates adding rules to the Delegate and setIntent() on an Indexer in a single transaction.

• DelegateManager constructor injects DelgateFactory

  Functions:

  • createDelegate()
    • returns a new Delegate by calling the DelegateFactory. Where the owner of the newly created Delegate would be the caller, msg.sender.
    • Upon creation of a new Delegate, the manager saves the address in the owner to list of delegates mapping.
  • constructor()
    • Upon creation of a new Delegate, the manager saves the address
  • setRuleAndIntent(address Delegate, struct Intent, struct Rule, address Indexer)
    • sets a Rule on a Delegate and then subsequently setIntent() on an Indexer in a single transaction.
  • unsetRuleAndIntent(address Delegate, struct Intent, struct Rule, address Indexer)
    • unsets a Rule on a Delegate and then subsequently unsetIntent() on an Indexer in a single transaction.

Other considerations:

• The DelegateManager is indifferent to the chosen Indexer it tries to set an intent on. The ultimate responsibility of successfully setting an intent is determined by the Indexer. This is because the Indexer references a LocatorWhitelist of which the DelegateFactory implements. If a provided DelegateManager cannot be verified to be from the trusted DelegateFactory the transaction will fail with LOCATOR_NOT_WHITELISTED.
• When setting an intent through the DelegateManager the msg.sender will always be the staker. The DelegateManager is only a facilitator to the Indexer.
• Multiple Delegates are akin to different strategies.

In reference to: https://github.com/airswap/airswap-protocols/pull/159/files#r335149519

Description:

Update Indexer.setIntent() and Delegate.setRule() methods to use structs instead of 3+ parameters.

Existing Functionality:

Currently Indexer.setIntent() and Delegate.setRule pass series of parameters in

 function setIntent(
    address _signerToken,
    address _senderToken,
    uint256 _amount,
    bytes32 _locator
  )

  function setRule(
    address _senderToken,
    address _signerToken,
    uint256 _maxSenderAmount,
    uint256 _priceCoef,
    uint256 _priceExp
  )

Proposed Functionality:

These parameters can be better represented as structs

 function setIntent(
    Struct.Intent _newIntent
  )

  function setRule(
    Struct.Rule _newRule
  )

• The new structures would exist within a Struct.sol file
• Struct.sol could be a library
• This would simplify the number of inputs

Reason:

The idea is that by passing them as objects it abstracts away the innards of the rule or intent.
With regards to gas passing many parameters downwards could be simplified, thus reducing the method signature and subsequently gas usage.

Considerations:

• Many tests need to be updated.
• Pass through methods of parameters would need to be updated.

Few potential flows ideas:

• Have two functions on swap to dictate whether ether or weth transaction
• Similar to the current swap contract V1:
  • Allow someone to send an order with ether and receives back some token
  • (V2) Allow a msg.sender who must then also be the _order.sender.wallet send ether to the Swap contract
  • Pass the ether to the signer and not do any conversion
• Slights deviation when a msg.sender sends ether and it gets converted to WETH when delivered to the _order.signer.wallet
  • Allow someone to send an order with ether and receive back some token
  • (V2) Allow a msg.sender who must then also be the _order.sender.wallet send ether to the Swap contract
  • Swap contract will deposit the ether to become WETH
  • Swap will move the WETH to the signer and transfer the tokens to the msg.sender (sender wallet)
  • no additional authorizations for sender/signer would be required for this to function
• Signer is sending back WETH and now convert it to ether before reaching the taker
  • Swap will be performed and the _signer.token will be checked to see if it's WETH which will kick off the flow
  • then the sender will need to authorize the swap contract to transfer WETH
  • It will move the WETH to the swap contract and then call weth.withdraw
  • Swap will then need a fallback function to receive the ether returned
  • Swap contract will transfer the ether to the msg.sender

Considerations

• Adds more complexity to swap which is the core piece of the swap protocol
• Reduces gas costs potentially (optimizer could help)
• Allow weth-to-weth trxns eth-to-weth trxn and weth-to-eth trxns since all seem like viable sequences dependent on the use case

Description

In reference to: https://www.notion.so/fluidity/Indexer-Audit-ef2f668a578f4fefa050442933683bac
Remove the array parameters in Indexer.addToBlacklist() and Indexer.removeFromBlacklist()

Existing Functionality

  function addToBlacklist(
    address[] calldata _tokens
  ) external onlyOwner {

  function removeFromBlacklist(
    address[] calldata _tokens
  ) external onlyOwner {

Proposed Functionality

  function addToBlacklist(
    address _tokenToAdd
  ) external onlyOwner {

  function removeFromBlacklist(
    address _tokenToRemove
  ) external onlyOwner {

Reason

We can't really imagine batch adding or removing blacklisted tokens. If we needed we can loop over the tokens.

Considerations

Explanation

Having done some reading and double-checked my findings with a quick code-check (https://github.com/airswap/airswap-protocols/tree/test-struct-duplication), I've found we're storing each locator twice in the Index contract.
E.g. if we have a simple linked list HEAD <-> ALICE <-> HEAD, we are storing:
linkedList[HEAD][NEXT] = ALICE
linkedList[HEAD][PREV] = ALICE
These are not stored as references (as sometimes structs are), and are instead stored as separate copies of the same locator.

This isn't a problem from a security point of view - its very clear in the code the 2 copies of a locator can never be updated differently and cause discrepancies. However it is clearly a waste of storage and therefore gas.

Gas calculation of current setup

To add a new locator to the linked list in the current setup, we have the following:
Storage before addition of BOB (who goes between ALICE and CHARLIE):

linkedList[ALICE][NEXT] = CHARLIE
linkedList[CHARLIE][PREV] = ALICE

Storage after the addition of BOB:

linkedList[ALICE][NEXT] = BOB     // update
linkedList[CHARLIE][PREV] = BOB   // update
linkedList[BOB][NEXT] = CHARLIE   // new
linkedList[BOB][PREV] = ALICE     // new

This change requires the storage of 2 new locators in storage, and 2 updated locators.
Locators are 3 'words' in size (they are 3 x 32bytes), meaning we have 6 new storage writes, and 6 updates storage slots.
It costs 20,000 to use a new 32 byte storage
It costs 5,000 to overwrite a previously used 32 byte storage

The addition of BOB to the linked list therefore costs (6x20000 + 6x5000) = 150,000

Gas calculation of new setup

The new linked list would have a mapping as follows:

mapping(address => ListElement) linkedList;
struct ListElement {
     address previous;
     address next;
     uint256 score;
     bytes32 data;
}

Note that this no longer has a address user; field as Locator did previously - it's not needed anymore.

Storage before addition of BOB (who goes between ALICE and CHARLIE):

linkedList[ALICE] = ListElement(p, CHARLIE, s, d)
linkedList[CHARLIE] = ListElement(ALICE, n, s, d)

Storage after addition of BOB:

linkedList[ALICE] = ListElement(p, BOB, s, d)        // `next` address updated
linkedList[CHARLIE] = ListElement(BOB, n, s, d)      // `previous` address updated
linkedList[BOB] = ListElement(ALICE, CHARLIE, s, d)  // new

This change requires 2 32 byte updates, plus one new struct (which is 4 x 32 bytes).

The addition of BOB to the linked list therefore costs (4x20000 + 2x5000) = 90,000

Propose that having multiple statuses for an order nonce is unnecessarily informative. We can reduce complexity and remove a require in Swap.sol with the following.

Currently:

• signerOrderStatus is either OPEN (0x0), TAKEN (0x0), or CANCELED (0x0)

Proposed:

• Rename signerOrderStatus to signerNonceStatus
• signerNonceStatus is either AVAILABLE (0x0) or UNAVAILABLE (0x1)

Purpose: Allow AirSwap to stay in control of deployed smart contracts in testing stages and remove them if issues are found, either through pausing or selfdestruct

Contracts to add these to:

• Wrapper
• Indexer
• ... others

Two functions to add:

• just put in PauserRole/ Pausable
• Second function to selfdestruct the contract if in a paused state

Purpose

Implement an indexer that enables multiple parties to stake for the same locator. This enables parties to pool their staking power to compete on an index.

The current Indexer is an "intent indexer" which creates many indexes based on signerToken senderToken pairs. The proposed design is a "multi-stake indexer" which manages a single index.

Why another indexer?

• Groups and communities can stake for software and services they like on the network.
• Operators of the software and services found at a locator can encourage others to stake.
• Rather than coupling "indexers of indexers" we wish to index applications that use indexers.

Requirements

• Any party can stake for any locator and the score of that locator increases accordingly.
• When a party unstakes, the score of that locator decreases accordingly.
• Until all parties have unstaked a locator, the locator persists on the index.
• The existing Index.sol should be able to remain unaltered.

Current Behavior

• The current behavior is that Indexers have an index for each token pair.
• An index holds a linked list of locators ordered by score.
• A user creates an Index by interacting with the Indexer and calling createIndex.
• Once an Index (linkedlist) is created, users can then call setIntent to add themselves to the Indexer.

Updated Behavior

High level: Keep the Index to be agnostic and do all the staking and staking management on the Indexer

Contract Index.sol

method for updateLocator(_user, _amount) called only by the Indexer by the additional staker

This will update the score as well as place the locator in the correct position

Only the indexer will be able to update these scores.

This will be part of topupDelegate and returnAmtDelegate

Contract Indexer.sol

stakingPatrons - name of 2-layer mapping

_address → Index() → _amt mapping to store how much each user staked

as well as array holding the patrons were Index

indexPatrons - name of mapping to array

Index() → [] so that when an intent is unset all the values are given back

add in two new function

topupDelegate(Index address, stake amount)

will both store the amount from the msg.sender and how much they staked

returnAmtDelegate(Index address, stake amount) or not have any stake amount and just remove value

More details

topupDelegate(Index address, stake amount)

• add to the stakingPatrons mapping
• adding to the indexPatrons at the end
• msg.sender needs to approve this Indexer contract to transfer AST on their behalf
• AST.transferFrom(msg.sender, address(this), amount)
• OPTIONAL bring the approve back to 0
• fetch that it's an existing index and not blacklisted
• call Index.updateLocator() which will update the score and position of the Locator

returnAmtDelegate(Index address, uint256 stakeamount)

• remove from stakingPatrons mapping if amount equals how much was staked
• remove from indexPatron if amount equals how much was staked
• AST.transfer(msg.sender, amount)
• fetch the exsiting index
• call Index.updateLocator() with a negative number to update the score to be negative

Consideration

• Lock the number of patrons possible to 10 to ensure when unsetting intents, the max transfer will be 10. The reason for limiting to 10 patrons is that AST transfers are about 50 K and so 10 of these would be 500K in gas. We would not want there to be too many patrons such that an intent cannot be withdrawn due to out of gas error.
• Tests to ensure that a patron cannot put the Delegate / Locator in a worse position then they had initially staked
• No dangling pointers possible
• all amounts of staking token must be non-zero outside of the initial makerDelegate take

Existing Functionality

Currently when setRuleAndIntent() is called staked tokens are provided by the msg.sender. Subsequently when unsetRuleAndIntent() is called, tokens are returned to the msg.sender.

Proposed Functionality

msg.sender and tradeWallet should both be options on where staking tokens are pulled and returned from. A defaultStakeAccount can be set which can either be msg.sender or tradeWallet. Both setRuleAndIntent() and unsetRuleAndIntent() would first check against the defaultStakeAccount to know where to pull/return funds from/to.

Change Required on Wrapper contract

• _order.maker.wallet is checked with _order.signature to ensure that the maker truly signed the order
• Use of msg.sender.call.value(amount)("") instead of transfer()
  • https://diligence.consensys.net/posts/2019/09/stop-using-soliditys-transfer-now/

Test case to add

it('Send order where Bob sends AST to Alice for DAI', async () => {
    const order = await orders.getOrder(
    {
    maker: {
    wallet: aliceAddress,
    token: tokenDAI.address,
    param: 1,
    },
    taker: {
    wallet: bobAddress,
    token: tokenAST.address,
    param: 100,
    },
    },
    true
    )
    
    order.signature = signatures.getEmptySignature()
    
      console.log(order)
    
      let result = await wrappedSwap(order, {
        from: bobAddress,
        value: 0,
      })
      await passes(result)
      result = await getResult(swapContract, result.tx)
      emitted(result, 'Swap')
      equal(await web3.eth.getBalance(wrapperAddress), 0)
      ok(
        await balances(wrapperAddress, [
          [tokenAST, 0],
          [tokenDAI, 0],
          [tokenWETH, 5],
        ])
      )
    })

Consequences of the Change

• More gas will be consumed as two hashes will be computed and two ecrecovers done