cylon56 / report-automation-action Goto Github PK

In the OrderMixin contract, the _makeCall function is used to transfer assets from the taker to the maker and then from the maker to the taker. In the latter transfer, the _makeCall function is incorrectly passed the order's makerAsset as the last parameter, when it should be the order's makerAssetData.

As a result, any proxy functionality that relies on the makerAssetData argument will break.

To be consistent with the earlier call to _makeCall and to fully support proxy functionality, consider updating the order.makerAsset parameter to order.makerAssetData.

Update: Fixed in pull request #57.

Orders from the OrderMixin contract have the ability to be partially filled. To support partial fills, the protocol requires a way to calculate both sides of swaps. Both getMakerAmount and getTakerAmount fields are defined by the maker of the order for this exact purpose.

When filling an order, takers must provide either the makingAmount or the takingAmount values as well as a thresholdAmount value. There are two different code-paths that can be taken, based on if the makingAmount or the takingAmount was provided.

The first one is when the makingAmount parameter is defined. It could truncate the makingAmount value and also calculate the takingAmount value for it. In this situation, the thresholdAmount ensures that the takingAmount value taken is not unexpectedly large.

The second one is when the takingAmount parameter is defined. In such case, it will calculate the makingAmount value, with the possibility of truncating it and recalculating the takingAmount value if that happens. In this situation, the thresholdAmount value ensures that the makingAmount value returned is not unexpectedly small.

There exist two exploitation methods, each unique to one of the previous mentioned code-paths. These exploitation methods require malicious getMakerAmount and getTakerAmount functions. A simple implementation of these functions would have an identical behavior to AmountCalculator's getMakerAmount and getTakerAmount functions, but with a hard-coded switch that will force them to return an attacker controlled value when needed.

The first, less severe exploit pattern involves the first code-path where the makingAmount value is specified in a fill order. A malicious maker would wait for a fill order which specifies makingAmount to show up in the mempool in order to frontrun it. They would drain all of the value except 1 from the maker's side and then force _callGetTakerAmount to return the amount specified in the user's thresholdAmount value (or their allowance if it is less). When the user's transaction finally goes through, they will swap their full thresholdAmount worth of takerAsset for a single unit of makerAsset. This exploit is limited by the amount given by the thresholdAmount value or the amount of the takerAsset the user allowed on the LimitOrderProtocol contract.

The second, more severe exploit pattern involves the second code-path where the takingAmount value is specified. The malicious maker would similarly wait for a fill order that specified a takingAmount value to show up in the mempool. They would frontrun the transaction and force the makingAmount value returned by _callGetMakerAmount function to be higher than both the remainingMakerAmount and the thresholdAmount. They would also set the takingAmount returned value by _callGetTakerAmount to be the amount of takerAsset asset allowed on the LimitOrderProtocol by the taker. When the taker's transaction goes through, it will truncate the makingAmount value and then recalculate the takingAmount value. This recalculation is not guaranteed to be lower however, and in this case will drain the taker of all the takerAsset that they allowed on the contract. In this code-path, the thresholdAmount value is ensuring that the makingAmount is not too low, so taking all the taker's takerAsset asset is unchecked. The funds lost are bounded by the amount of the takerAsset asset the user allowed on the LimitOrderProtocol contract.

These exploits are not possible without partial orders and, more specifically, partial orders with malicious getMakerAmount and getTakerAmount implementations.

The main issue of the thresholdAmount value check is that it only covers one side of the swap, but the other side can be manipulated via frontrunning. There are no assurances that the value the taker originally proposed remains unchanged. Consider removing makingAmount truncation from both code-paths and reverting if the order cannot support a fill as large as requested. By doing this, the thresholdAmount can be used to sufficiently restrict the other side of the swap and avoid unexpected behavior, even in malicious orders.

Update: Fixed in pull request #83.

The protocol allows the creation of private and public orders. On private orders, only the allowedSender address, specified by the maker during the order's creation, is able to fill the order.

However, in the OrderMixin contract, validation for the allowedSender address is incorrectly scoped, meaning that it is only evaluated inside the logic that handles the first fill of an order. If a private order is partially filled, then the check for the allowedSender address is no longer reachable and the order becomes fillable by anyone.

To clarify intent around whether any user should be able to fill partially-filled private orders or not, consider either documenting the reason for the current behavior or validating the allowedSender address outside of the scope of the first fill to ensure that it will be validated every time a fill is attempted.

Update: Fixed in pull request #58.

When users deposit some amount of asset into a vault, their newly deposited asset are considered pending until the beginning of the vault's next round. While pending, deposits are not being actively managed by the vault, and they are, appropriately, intentionally disregarded for many of the vaults internal accounting purposes. Pending deposits do not share in profits or losses of the vault while pending and they should not be included in the determination of the vault's price per share. Vaults keep a running count of the total pending asset in the vaultState.totalPending state variable so that they can actively disregard all of the pending asset where appropriate, and also so that they can mark all of the pending asset as active when the next round starts.

Delta vaults provide a withdrawInstantly function, which permits users to withdraw asset, including any pending asset, at any time. However, this function fails to update vaultState.totalPending, even after the user has withdrawn some amount of pending asset. As a consequence, for any sequence of deposits that are followed by instant withdrawals during the same round, vaultState.totalPending will only ever increase. This results in a mismatch between the number that is stored in vaultState.totalPending and the actual amount of pending asset the vault possesses.

This has far-reaching ramifications, because so many of the calculations in the vault are affected, including:

• The balance calculation within the pricePerShare function of RibbonVault
• The pps calculation in accountVaultBalance of RibbonVault
• The roundStartBalance calculation in the rollover function of VaultLifecycle

In many cases, OpenZeppelin's SafeMath is used to subtract the value of vaultState.totalPending from the total asset balance held by the vault, to exclude the former from being counted as active. This approach is reasonable and accurate, as long as vaultState.totalPending reflects the actual amount of pending asset in the vault. However, since vaultState.totalPending is never decremented when pending asset is withdrawn, it can become larger than the total balance. When this happens, many of the internal arithmetic operations of the vault overflow and then revert.

In this scenario, users can still withdraw funds, but the vault cannot carry out many other essential functions, including rolling to the next option. The only way to recover complete functionality would be to replace the logic of the Delta vault via an upgrade.

It is important to note that this critical loss of functionality can easily arise as a result of normal vault activity.

However, there is another scenario that could play out that stems from the failure to decrement vaultState.totalPending correctly. That scenario would probably be less likely to happen during the normal operation of the vault, but it can result in all user funds being locked in the vault. It would be trivial for a malicious party to put the vault in such a state.

In fact, this state can easily be achieved by performing the following steps immediately after an oToken auction ends:

1. Deposit an amount of asset equal to the current balance held by the vault (using a flash loan if needed).
2. Perform an instant withdrawal of all the funds that were just deposited. Then, because of the failure to decrement, vaultState.totalPending would be equal to the current balance of the vault.
3. Call the claimAuctionOtokens function, which would force a reevaluation of the price per share because of its updatePPS modifier.

While updating the price per share, the roundStartBalance would incorrectly be set to 0 because of the subtraction of pendingAmount from the currentBalance of the vault. When the modifier calls the getPPS function in VaultLifecycle, the price per share is derived from a multiplication of the roundStartBalance, always returning 0 under these conditions.

Whenever roundPricePerShare is equal to 0, withdrawals will fail due to require statements in the ShareMath library's underlyingToShares and sharesToUnderlying functions that are called as part of every withdrawal. All user funds will be locked in the vault.

Consider decrementing vaultState.totalPending correctly whenever any amount of pending asset is withdrawn from a vault. Also consider expanding the test suite to cover more complicated deposit and withdrawal .

Update: Fixed in commit f882323642c23f02965adfa378a6b062e0ca65ce of PR#79.

The AggregatorInterface interface appears to be a subset of code copied from ChainLink's public code repository. The full interface is included in ChainLink's contract npm package.

When possible, to lessen the potential for interface mismatches and resultant issues, rather than re-defining and/or rewriting another project's interfaces, consider using interfaces installed via their official npm packages instead.

Update: Fixed in pull request #66.

There are instances of duplicated code within the codebase. Duplicating code can lead to issues later in the development lifecycle and leaves the project more prone to the introduction of errors. Such errors can inadvertently be introduced when functionality changes are not replicated across all instances of code that should be identical. Examples of duplicated code include:

• In the Permitable contract, the _permit and _permitMemory functions are duplicates.
• The calculations in the getMakerAmount and getTakerAmount functions are duplicated in the fillOrderRFQTo function when calculating making and taking amounts, respectively.

Rather than duplicating code, consider having just one contract or library containing the duplicated code and using it whenever the duplicated functionality is required.

Update: Partially fixed in pull request #60.

blah blah

There are a few occurrences of literal values being used with unexplained meaning in the codebase. For example:

• In the OrderMixin contract, the _remaining mapping is semantically overloaded (as explained in the issue Semantic overloading of mapping) to track the amount of asset remaining for a partially filled order as well as if an order has been completely filled. Specifically, 0 means that no fills associated with an order have been made, 1 means an order can no longer be filled, and anything larger than 1 means that there is a remaining amount associated with the order that can potentially be filled.
• In the ChainlinkCalculator contract, the literal value 1e18 is used in the singlePrice function.

To improve the code's readability and facilitate refactoring, consider defining a constant for every magic number, giving it a clear and self-explanatory name. For complex values, consider adding an inline comment explaining how they were calculated or why they were chosen.

Update: Fixed in pull request #75 and pull request #76.

Throughout the codebase, a few instances of misleading and/or incomplete inline documentation were identified and should be fixed.

The following are instances of misleading inline documentation:

• In the ChainlinkCalculator contract, the singlePrice function's NatSpec @notice tag says that it Calculates price of token relative to ETH scaled by 1e18, but in fact, its result is the value of amount tokens scaled by 1e18, where the oracle may not report in terms of ETH (for a pair not including ETH, for instance).
• In the OrderRFQMixin contract, the invalidatorForOrderRFQ function's NatSpec @return tag is misleading, because the quote may not have been filled for the respective invalidator bit to have been set. The order can also have been canceled.

The following are instances of incomplete inline documentation:

• Functions in the AmountCalculator contract do not describe any of the parameters.
  • Also worth noting is that this is a code example in a nested list.
• In the ChainlinkCalculator contract, the singlePrice and doublePrice functions do not describe all of the parameters.
  • Also worth noting is that this is a code example in a nested list.
• In the ImmutableOwner contract, the public variable and modifier have no NatSpec.
• In the InteractiveNotificationReceiver contract, the notifyFillOrder function does not describe any of the parameters.
  • Also worth noting is that this is a code example in a nested list.
• In the LimitOrderProtocol contract, the DOMAIN_SEPARATOR function has no NatSpec.

Clear inline documentation is fundamental for outlining the intentions of the code. Mismatches between the inline documentation and the implementation can lead to serious misconceptions about how the system is expected to behave. Consider fixing these errors to avoid confusion for developers, users, and auditors alike.

Update: Partially fixed. Misleading documentation addressed in pull request #75 and pull request #77.

The 1inch team states:

We've fixed misleading docs. Completion of the docs will be done later.

During the installation of the project's dependencies, NPM warns that one of the packages installed, Highlight, "will no longer be supported or receive security updates in the future".

Even though it is unlikely that this package could cause a security risk, consider upgrading the dependency that uses this package to a maintained version.

Update: Fixed in pull request #64.

Throughout most of the codebase, the protocol explicitly makes external calls via OpenZeppelin's functionStaticCall method to restrict the possibility for state changes when they are either not expected or not desirable. However, in the ChainlinkCalculator contract, despite the intention of making external calls only to view methods on Chainlink oracles, the external calls in the singlePrice and doublePrice functions are not made via explicit staticcalls.

While we did not identify any immediate security concerns stemming from this, to reduce the attack surface, improve consistency, and clarify intent, consider using explicit staticcalls, for all external calls to view functions in the ChainlinkCalculator contract.

Update: Not fixed. The 1inch team states:

We think that syntax complication nullifies improvements in consistency.

In the OrderMixin contract, the getTakerAmount and getMakerAmount bytes fields are used as arguments for the _callGetTakerAmount and _callGetMakerAmount functions. These calls provide a way to calculate one side of the swap based on the other side, and they allow users to partially fill orders.

The getTakerAmount/getMakerAmount fields are dynamic variables and are packed in front of the takerAmount and makerAmount values in the _callGetTakerAmount and _callGetMakerAmount functions. It is possible for a malicious maker to provide more data than expected in the getTakerAmount and
getMakerAmount fields to push the takerAmount and makerAmount bytes past where they are assumed to be when being decoded in the next function. This allows the maker to shift the passed in taker or maker amount by a full bytes to the right and even replace them completely if an extra 32 bytes of data is provided.

Users already have to manually review the getTakerAmount and getMakerAmount fields in the order, but this technique is rather hard to spot. Also worth noting, this attack even applies to the internally trusted getMakerAmount and getTakerAmount functions. For most attacks, providing a reasonable threshold amount will prevent loss of funds.

To prevent this, consider encoding the static arguments before the dynamic arguments to avoid giving the dynamic arguments a method to control the static arguments.

Update: Not fixed. The 1inch team stated:

We'll take extra care with getters validation. We'll try to implement sanity validation of getters in our sdk that will help with filtering potentially malicious orders.

The codebase contains the following typos:

• On line 18 of OrderMixin.sol and line 11 of OrderRFQMixin.sol, v1 should be v2.
• On lines 147, 165, and 188 of OrderMixin.sol, it's should be if it's.

Additionally the project's README (out of scope for this audit) contains the following typos:

• Ket should be Key.
• stategies should be strategies.
• cancelation should be cancellation.

Consider correcting these typos to improve code readability.

Update: Fixed in pull request #71 and pull request #77.

This is not a real finding. It is used for testing the report template.

Here is a test of a footnote!!footnote|First footnote!!. And another!!footnote|Second footnote!! footnote for fun.

We don't typically include inline code, but it would be nice if we supported it. Below are several examples.

Solidity, the most useful for us:

// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts v4.4.0 (token/common/ERC2981.sol)

pragma solidity ^0.8.0;

import "../../interfaces/IERC2981.sol";
import "../../utils/introspection/ERC165.sol";

abstract contract ERC2981 is IERC2981, ERC165 {
    struct RoyaltyInfo {
        address receiver;
        uint96 royaltyFraction;
    }

    RoyaltyInfo private _defaultRoyaltyInfo;
    mapping(uint256 => RoyaltyInfo) private _tokenRoyaltyInfo;

    /**
     * @dev See {IERC165-supportsInterface}.
     */
    function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) {
        return interfaceId == type(IERC2981).interfaceId || super.supportsInterface(interfaceId);
    }

    /**
     * @inheritdoc IERC2981
     */
    function royaltyInfo(uint256 _tokenId, uint256 _salePrice)
        external
        view
        virtual
        override
        returns (address, uint256)
    {
        RoyaltyInfo memory royalty = _tokenRoyaltyInfo[_tokenId];

        if (royalty.receiver == address(0)) {
            royalty = _defaultRoyaltyInfo;
        }

        uint256 royaltyAmount = (_salePrice * royalty.royaltyFraction) / _feeDenominator();

        return (royalty.receiver, royaltyAmount);
    }

    /**
     * @dev The denominator with which to interpret the fee set in {_setTokenRoyalty} and {_setDefaultRoyalty} as a
     * fraction of the sale price. Defaults to 10000 so fees are expressed in basis points, but may be customized by an
     * override.
     */
    function _feeDenominator() internal pure virtual returns (uint96) {
        return 10000;
    }

    /**
     * @dev Sets the royalty information that all ids in this contract will default to.
     *
     * Requirements:
     *
     * - `receiver` cannot be the zero address.
     * - `feeNumerator` cannot be greater than the fee denominator.
     */
    function _setDefaultRoyalty(address receiver, uint96 feeNumerator) internal virtual {
        require(feeNumerator <= _feeDenominator(), "ERC2981: royalty fee will exceed salePrice");
        require(receiver != address(0), "ERC2981: invalid receiver");

        _defaultRoyaltyInfo = RoyaltyInfo(receiver, feeNumerator);
    }

    /**
     * @dev Removes default royalty information.
     */
    function _deleteDefaultRoyalty() internal virtual {
        delete _defaultRoyaltyInfo;
    }

    /**
     * @dev Sets the royalty information for a specific token id, overriding the global default.
     *
     * Requirements:
     *
     * - `tokenId` must be already minted.
     * - `receiver` cannot be the zero address.
     * - `feeNumerator` cannot be greater than the fee denominator.
     */
    function _setTokenRoyalty(
        uint256 tokenId,
        address receiver,
        uint96 feeNumerator
    ) internal virtual {
        require(feeNumerator <= _feeDenominator(), "ERC2981: royalty fee will exceed salePrice");
        require(receiver != address(0), "ERC2981: Invalid parameters");

        _tokenRoyaltyInfo[tokenId] = RoyaltyInfo(receiver, feeNumerator);
    }

    /**
     * @dev Resets royalty information for the token id back to the global default.
     */
    function _resetTokenRoyalty(uint256 tokenId) internal virtual {
        delete _tokenRoyaltyInfo[tokenId];
    }
}

Here is some Yul:

{
    let x := 0
    switch calldataload(4)
    case 0 {
        x := calldataload(0x24)
    }
    default {
        x := calldataload(0x44)
    }
    sstore(0, div(x, 2))
}

TypeScipt, and Javascript for syntax highlighting should be very similar:

#!/usr/bin/env node

const { promises: fs } = require('fs');
const path = require('path');

const pathUpdates = {
  'access/TimelockController.sol': 'governance/TimelockController.sol',
  'cryptography/ECDSA.sol': 'utils/cryptography/ECDSA.sol',
  'cryptography/MerkleProof.sol': 'utils/cryptography/MerkleProof.sol'
};

async function main (paths = [ 'contracts' ]) {
  const files = await listFilesRecursively(paths, /\.sol$/);

  const updatedFiles = [];
  for (const file of files) {
    if (await updateFile(file, updateImportPaths)) {
      updatedFiles.push(file);
    }
  }

  if (updatedFiles.length > 0) {
    console.log(`${updatedFiles.length} file(s) were updated`);
    for (const c of updatedFiles) {
      console.log('-', c);
    }
  } else {
    console.log('No files were updated');
  }
}

module.exports = {
  pathUpdates,
  updateImportPaths,
  getUpgradeablePath,
};

if (require.main === module) {
  const args = process.argv.length > 2 ? process.argv.slice(2) : undefined;
  main(args).catch(e => {
    console.error(e);
    process.exit(1);
  });
}

Rust:

// Copyright 2015-2020 Parity Technologies (UK) Ltd.
// This file is part of OpenEthereum.

// OpenEthereum is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// OpenEthereum is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with OpenEthereum.  If not, see <http://www.gnu.org/licenses/>.

extern crate ethjson;
extern crate serde_json;

use ethjson::spec::Spec;
use std::{env, fs, process};

fn quit(s: &str) -> ! {
    println!("{}", s);
    process::exit(1);
}

fn main() {
    let mut args = env::args();
    if args.len() != 2 {
        quit(
            "You need to specify chainspec.json\n\
		\n\
		./chainspec <chainspec.json>",
        );
    }

    let path = args.nth(1).expect("args.len() == 2; qed");
    let file = match fs::File::open(&path) {
        Ok(file) => file,
        Err(_) => quit(&format!("{} could not be opened", path)),
    };

    let spec: Result<Spec, _> = serde_json::from_reader(file);

    if let Err(err) = spec {
        quit(&format!("{} {}", path, err.to_string()));
    }

    println!("{} is valid", path);
}

Go:

// Copyright 2018 The go-ethereum Authors
// This file is part of the go-ethereum library.
//
// The go-ethereum library is free software: you can redistribute it and/or modify
// it under the terms of the GNU Lesser General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// The go-ethereum library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.

package rpc

import (
	"context"
	"errors"
	"io"
	"net"
	"os"
	"time"
)

// DialStdIO creates a client on stdin/stdout.
func DialStdIO(ctx context.Context) (*Client, error) {
	return DialIO(ctx, os.Stdin, os.Stdout)
}

// DialIO creates a client which uses the given IO channels
func DialIO(ctx context.Context, in io.Reader, out io.Writer) (*Client, error) {
	return newClient(ctx, func(_ context.Context) (ServerCodec, error) {
		return NewCodec(stdioConn{
			in:  in,
			out: out,
		}), nil
	})
}

type stdioConn struct {
	in  io.Reader
	out io.Writer
}

func (io stdioConn) Read(b []byte) (n int, err error) {
	return io.in.Read(b)
}

func (io stdioConn) Write(b []byte) (n int, err error) {
	return io.out.Write(b)
}

func (io stdioConn) Close() error {
	return nil
}

func (io stdioConn) RemoteAddr() string {
	return "/dev/stdin"
}

func (io stdioConn) SetWriteDeadline(t time.Time) error {
	return &net.OpError{Op: "set", Net: "stdio", Source: nil, Addr: nil, Err: errors.New("deadline not supported")}
}

Python:

#!/usr/bin/python3

import sys
from markdown import markdownFromFile
import config

def markdown_to_html(markdown_path, html_path):
    
    markdownFromFile(
        input=markdown_path,
        output=html_path,
        output_format='html5',
        extensions=config.MARKDOWN_EXTENSIONS,
    )

if __name__ == '__main__':
    if len(sys.argv) != 3:
        print("Usage: markdown_to_html.py path/to/markdown.md path/to/output.html")
        exit(1)
    
    # Parse args
    markdown_path = str(sys.argv[1])
    output_path = str(sys.argv[2])

    markdown_to_html(markdown_path, output_path)

Certain data formats might be important for us, just as JSON, YAML, or TOML.

Here is a JSON example:

{
  "name": "openzeppelin-solidity",
  "description": "Secure Smart Contract library for Solidity",
  "version": "4.4.1",
  "files": [
    "/contracts/**/*.sol",
    "/build/contracts/*.json",
    "!/contracts/mocks/**/*"
  ],
  "bin": {
    "openzeppelin-contracts-migrate-imports": "scripts/migrate-imports.js"
  },
  "repository": {
    "type": "git",
    "url": "https://github.com/OpenZeppelin/openzeppelin-contracts.git"
  },
  "keywords": [
    "solidity",
    "ethereum",
    "smart",
    "contracts",
    "security",
    "zeppelin"
  ],
  "author": "OpenZeppelin Community <[email protected]>",
  "license": "MIT",
  "bugs": {
    "url": "https://github.com/OpenZeppelin/openzeppelin-contracts/issues"
  },
  "homepage": "https://openzeppelin.com/contracts/"
}

YAML below, and TOML is pretty similar in syntax and formatting:

comment: off
github_checks:
  annotations: false
coverage:
  status:
    patch:
      default:
        target: 95%
    project:
      default:
        threshold: 1%

Update: We always include some update at the end, an update or reply from the client. It typically includes links to fixes, sometimes with inline code format.
Often, a quote from the client is included:

Some explanation for why this is code is not fixed.
Combining italics and quote blocks in markdown can be tricky.

Getting this quote on multiple lines is a chore. What if it also includes inline code? Finally, here's another small code block:

modifier onlyOwner() {
    require(owner() == _msgSender(), "Ownable: caller is not the owner");
    _;
}

The OrderMixin contract allows maker users to submit permitable orders so those can be executed in one transaction, rather than having to have a separate transaction for approvals. Also, order takers can submit their own permit during the filling of the order for the same purpose.

However, because the maker's permit is contained inside the order, both the maker's and the taker's permits would be accessible while the order-fill transaction is in the mempool. This would make it possible for any malicious user to take those permits and execute them on the respective asset contracts while frontrunning the fill transaction. Because these permits have a nonce to prevent a double spending attack, the order's fill transaction would fail as a result of trying to use the same permit that was just used during the frontrun.

Although there is no security risk, and the maker could create a new order and pre-approve the transaction, this attack could certainly impact the usability of permitable orders. Indeed, a motivated attacker could block all permitable orders with this attack. Consider validating if the permit was already submitted, or if the allowance is enough, during the order fills. Also consider letting users know about this possible attack during order composition.

Update: Not fixed. The 1inch team states:

We had approval checks before but decided to simplify permit flow to just revert on unsuccessful approvals. We'll think about the ways to notify makers about the issue.

The LimitOrderProtocol contract inherits the ChainlinkCalculator contract through the OrderMixin contract. This contract exposes two functions to enable the usage of Chainlink oracles during the predicates check and the lookup of the maker amount/taker amount.

However, the contract makes undocumented assumptions about the number of decimals that the Chainlink oracles should report in, as well as the number of decimals that the function parameters should contain. In certain scenarios, this could lead to unexpected behaviors, including the mis-pricing of assets and the unintentional loss of funds.

More specifically, throughout the contract the implicit assumption is that the Chainlink oracles will report with 18 decimals of precision. However, not all Chainlink oracles report with this number of decimals. In fact, if the oracle reports a token pair that is in terms of a currency (USD, for instance), it will only have 8 decimals of precision. Since there are no restrictions on which oracles can be used, implicit assumptions should not be made about the number of decimals they will report with.

Relatedly, there is an implicit assumption that the amount parameter for the ChainlinkCalculator functions will use 18 decimals, together with the misleading explicit declaration that the singlePrice function Calculates price of token relative to ETH scaled by 1e18. In reality, even with an oracle that does report with 18 decimals, the return value of the singlePrice function would be scaled by the number of decimals of the amount parameter, which may not necessarily be 18 decimals.

Similarly, the doublePrice function assumes that two Chainlink oracles will report with the same number of decimals, causing the result of the function to deviate from expectations.

Consider explicitly documenting assumptions regarding the number of decimals that parameters and return values should be in terms of. Furthermore, consider either limiting calculations that depend on oracles that break those assumptions, or having the relevant calculations take the actual number of decimals into account.

Update: Fixed in pull request #75.

It is possible to exchange more than just ERC20s via the OrderMixin by deploying a contract that shares the same function selector as IERC20's transferFrom, and providing that contract as the makerAsset or the takerAsset in an order.

The out-of-scope proxies, namely, ERC721Proxy, ERC721ProxySafe, and ERC1155Proxy contracts follow this pattern to provide support for ERC721 and ERC1155 tokens. Since the proxies must be called with the same pattern as an IERC20 transferFrom call, the signature must start with address from, address to and uint256 amount. Anything else that the proxies require can be passed in after, and is defined in the order as makerAssetData and takerAssetData.

ERC1155s can naturally transfer multiple of the same id tokens at once, which means the ERC1155Proxy contract makes use of the amount field. On the other hand, ERC721s do not have an obvious use for the amount field. Since they represent non-fungible tokens, a specific tokenId will only have one in existence, rendering the amount field useless. Because of this, the implementation for both ERC721Proxy and ERC721ProxySafe contracts use the required amount field as the tokenId instead.

This overloading of the amount parameter creates the possibility of partially filling ERC721 orders in order to purchase separately listed tokens at discounted prices. For instance, there could be a case where a single user has multiple ERC721s of the same contract permitted to be transferred by the ERC721Proxy contract and lists them in separate limit orders.
If the limit orders also provide the getMakerAmount and getTakerAmount fields, it will be possible to partially fill these ERC721 orders. Since the order's amount field actually corresponds to the tokenId, a malicious user can place a partial fill on the ERC721 with the higher tokenId, resulting in a makingAmount/takingAmount of an ERC721 that could correspond to a lower tokenId. The result is the ERC721 with the lower tokenId would be transferred at the price of (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id).

This exploit has a few requirements:

• Multiple ERC721s from the same contract to be allowed on either ERC721 proxy by a single owner.
• Open order for one of the ERC721s that is not the lowest tokenId of the ones allowed.
• Partial fills allowed on the order.

To completely remove the possibility of partial ERC721 fills, consider separating the amount and tokenId arguments. Whether the arguments are separated or not, consider also documenting this to alert users of this behavior and to avoid this pattern in the future.

Update: Fixed in pull request #59.