Skip to content
Atomic Loans audit report
Branch: master
Clone or download
Fetching latest commit…
Cannot retrieve the latest commit at this time.
Permalink
Type Name Latest commit message Commit time
Failed to load latest commit information.
static-content initial report Jul 22, 2019
tool-output initial report Jul 22, 2019
README.md final report Aug 16, 2019

README.md

Atomic Loans Audit

1 Summary

ConsenSys Diligence conducted a security audit on the Atomic Loans smart contract system.

  • Project Name: Atomic Loans
  • Client Name: Atomic Loans
  • Client Contact: Matthew Black
  • Lead Auditor: Steve Marx
  • Co-auditors: Dean Pierce
  • Date: 2019-07-22

2 Audit Scope

This audit covered the following files:

File Name SHA-1 Hash
bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js de59246dd9597d27e81158c93c3ec6724f44cd26
bitcoin/packages/bitcoin-collateral-provider/lib/BitcoinCollateralProvider.js 4aee6ed25fe919e5a9055db45e306e20cf2d85b1
bitcoin/packages/bitcoin-collateral-swap-provider/lib/BitcoinCollateralSwapProvider.js 90a25193f0f93b3d895d42b8bd53bf1245cc6183
ethereum/contracts/BTCCurrency.sol 078298ff19c89d40ede080ec0b01c5bb6dc151ef
ethereum/contracts/Currency.sol f76dfbdf849e14202af874b7a3e96e244775de6d
ethereum/contracts/DSMath.sol 2a06c744883278ceb39e54b558395359454e519f
ethereum/contracts/Funds.sol a2de9e98ee0679061b795f8146aeaf0a48a43d26
ethereum/contracts/Loans.sol 72400480e986cbf206ef249bd14aca6c28833df3
ethereum/contracts/Medianizer.sol 31888451070ff075b4d7fa3d30fc0177a2bd4cfc
ethereum/contracts/Migrations.sol e8fc04f294cacc963593a12ee2c73058f6d1128a
ethereum/contracts/Sales.sol 7df226f3d981a115f8447f6ebede989399182649
ethereum/contracts/Vars.sol 1c558537bd6bd640b03676044787d53759ddb8c5
oracles/contracts/Buffer.sol c9279716c2b0411f919d07b5803b1ecae38cc8f5
oracles/contracts/DSMath.sol c7251d9017d4ed859e91bcef45d4f5c1f454de03
oracles/contracts/DSValue.sol a2f3f9f730319355c782053c04886e47138f76b6
oracles/contracts/ERC20.sol 45b7754c1112312c37054ab2fab1e71bd27fb3b4
oracles/contracts/Medianizer.sol 6b1f2ad14ed66e137f194fde19158b15145a9ac6
oracles/contracts/Migrations.sol e61057a352ae8d823d9a67ba231bed8a3017626f
oracles/contracts/Oracle.sol 44d0e4aacb2b65e0b194511a90cf4f4a5a881ee0
oracles/contracts/WETH.sol ebc95c26a2aa6d8a07d2a96a028cea97c297bd04
oracles/contracts/WETH9.sol ee3e6250a8886a1d9327acb960fd444b0e379d47
oracles/contracts/chainlink/BlockchainInfo.sol 56a6c98001f1a28db4ad4a4e6c363a78f06f5957
oracles/contracts/chainlink/ChainLink.sol 44fc5b7893b21b7872598c8052e7af7d3b7c9598
oracles/contracts/chainlink/CoinMarketCap.sol 278b8e73b10cf7b649625fbbf6663deb7ed14d93
oracles/contracts/chainlink/CryptoCompare.sol 0d78da1b5fab6e444b6e1bc7166fa09fa1b9a448
oracles/contracts/chainlink/Gemini.sol 9d608b02ede70feb35b8e178a331b801dd52a5ce
oracles/contracts/chainlink/SoChain.sol b52b37dcc135a4ec8ef6f7e6ce0c1b83f351287a
oracles/contracts/oraclize/Bitstamp.sol 95fa36649693685ac78fa52cd0ad3a3c3695f645
oracles/contracts/oraclize/Coinbase.sol db5baacebaf3fd36e966e95dee7022bdf9c56fa2
oracles/contracts/oraclize/Coinpaprika.sol c0d8a92558690d8b041ce26dff28a26ee2b81d74
oracles/contracts/oraclize/CryptoWatch.sol 1c49d7c049855f4736a077c4e9b443f6e225d0e3
oracles/contracts/oraclize/Kraken.sol 001df0bdcda5f095ddb3844ebafcdb0c1c2b22b8
oracles/contracts/oraclize/Oraclize.sol c2da807c0fc1c6ab9a9db2fa45954cd0bbff1d1c

(For the JavaScript files, only the Bitcoin scripts were audited.)

The audit team evaluated that the system is secure, resilient, and working according to its specifications. The audit activities can be grouped into the following three broad categories:

  1. Security: Identifying security related issues within the contract.
  2. Architecture: Evaluating the system architecture through the lens of established smart contract best practices.
  3. Code quality: A full review of the contract source code. The primary areas of focus include:
    • Correctness
    • Readability
    • Scalability
    • Code complexity
    • Quality of test coverage

3 System Overview

Atomic Loans provides a mechanism for cross-chain collateralized loans. Specifically, the system allows people to borrow Ethereum-based USD stablecoins while locking Bitcoin as collateral. In case of default, an auction is used to liquidate the collateral and make the lender whole. As a fallback for failed auctions, the lender can eventually seize a portion of the collateral.

3.1 Detailed Design

In the description below, we'll use the following characters:

  • Alice, who wishes to borrow Ethereum tokens using Bitcoin as collateral.
  • Bob, who wishes to lend out his Ethereum tokens.
  • Carol, an automated agent who can act on behalf of Alice or Bob when they are unavailable.
  • David, the highest bidder in a collateral liquidation auction.

Loan Establishment

To set up a loan, Alice and Bob must first agree on the loan parameters. This agreement can occur off chain, though there is an optional Funds contract that allows lenders to establish parameters on chain and accept loans from anyone who agrees to those parameters. These parameters include the amount of collateral and principal, the interest rate, what portion of the collateral can be seized in the case of default, durations for the various steps of the process, etc.

Part of setting up a loan is the establishment of secrets that will be used throughout the loan process. These secrets are used for a number of commit-reveal schemes, so the secrets are initially hashed and then revealed at the appropriate time.

  • A1 is revealed by Alice when she retrieves the loan principal. It can later be used by Bob to seize collateral once the loan has expired.
  • B1 is revealed by Bob to accept loan repayment or to cancel the loan (before Alice has received the principal). Alice can use this secret to reclaim her collateral.
  • A2 and B2 are used as part of the collateral liquidation process in case of default. There are actually multiple A2s and B2s to support multiple auctions.

To establish the loan, Alice must lock her collateral in a pair of Bitcoin P2SHs (pay to script hashes): one for the refundable collateral and one for the seizable collateral. At a high level, refundable collateral goes back to Alice unless it's sold in a liquidation auction, and seizable collateral may be seized by Bob if he is not repaid otherwise.

Once Bob is satisfied with the locked collateral, he releases the principal to Alice on the Ethereum blockchain. Alice takes this collateral, revealing A1.

Loan Repayment

Alice can repay the loan principal plus interest on the Ethereum blockchain. Bob must reveal B1 to accept this repayment, at which point Alice can unlock all of her collateral, and the loan is complete.

Collateral Liquidation

Alice may fail to repay the loan by the end of the loan period, or Bob may fail to accept repayment. In those cases, Alice's collateral is liquidated in an open auction. Of the funds raised in the auction, Bob receives up to the amount he's owed (principal plus interest), and Alice receives the remainder. The collateral is then paid to the auction's high bidder.

Alice's collateral is subject to liquidation in one other case. One of the loan's parameters is a minimum collateralization ratio, which is measured by determining the value of Alice's collateral in USD and comparing it to the amount of the loan. If the collateralization ratio is not maintained, a liquidation auction may be started even before the end of the loan period.

The auction process is complicated by the need to handle cross-chain interactions. The following is a description of the auction process:

  1. As bidders place their bids, they provide the hash of a secret D1, which locks their bid in the smart contract.
  2. At the end of the auction, the high bidder (David) is established.
  3. Alice and Bob produce signatures to move all of the collateral into new collateral swap P2SHs.
  4. Alice and Bob also produce signatures to move the collateral back to standard refundable collateral and seizable collateral scripts after a timeout. This is to handle the case where David never reveals D1.
  5. Alice and Bob, once they're both satisfied with the signatures, reveal A2 and B2.
  6. The collateral P2SHs accept Alice and Bob's signatures, along with A2 and B2, and allow funds to be moved to a new collateral swap script, locked with D1.
  7. David retrieves the Bitcoin collateral, revealing D1 in the process.
  8. Alice and Bob can use D1 to disburse the funds on Ethereum that David bid.

If Alice and Bob fail to move the collateral, David can request a refund of his bid after a timeout.

If David fails to reveal D1 in the time period alotted, Alice and Bob can move the funds back to standard collateral P2SHs (using the "back" scripts established in step 4) and attempt another auction.

Collateral Seizure

For an auction to be successful, both Alice and Bob have to participate by producing signatures and revealing secrets. Up to three auctions can occur. If all of them fail, Bob can seize the seizable portion of the collateral, and Alice can reclaim the refundable portion.

Using an Automated Agent

Many of the steps in the loan process have a liveness requirement. Alice or Bob need to be online to execute their part of the process before a timeout elapses. As an optional component, Alice and Bob can agree on an agent named Carol. Carol can act on behalf of either Alice or Bob at two key times:

  1. Either Bob or Carol can accept loan repayment or cancel a loan. This is particularly useful for lenders who may be offline at the time a borrower requests a loan.
  2. During an auction, it takes only two out of Alice, Bob, and Carol to move funds to the collateral swap script or to move them back after the sale has expired. This means either party can be offline during the auction.

3.2 Further Reading

The following resources are outdated but provide more information about the loan process and rationale:

  1. Atomic Loans: Cryptocurrency Debt Instruments
  2. BIP 197 - Hashed Time-Locked Collateral Contract
  3. ERC 1850 - Hashed Time-Locked Principal Contract Standard
  4. AtomicLoans.io

4 Key Observations/Recommendations

  • The separation of Loans, Funds, and Sales contracts is clean and helps to isolate unrelated concepts within the system.
  • The protocol is necessarily complicated due to the cross-chain interactions. This makes the system hard to understand and increases the risk of bugs.
  • Many variable and function names are quite unclear. A glossary is necessary to understand their purpose, and this makes reading the code very difficult. This is a problem both for auditors and for users of the system who want to understand what the code is doing.
  • The Ethereum contracts comingle data and funds for many different users. Several of the vulnerabilities discovered are considerably worse because of this. If, for example, each loan were handled by its own contract, a bug like issue 6.7 would have minimal impact.
  • A great deal of trust, probably more than most participants will realize, is placed in the automated agent Carol. Carol can unilaterally steal from Bob if he is using a fund, and she and Bob can collude to steal from Alice. She can also, through inaction, cause undesirable outcomes for Alice or Bob if they were relying on her to act before a timeout.
  • Testing is inadequate. For a contract system that is intended to handle significant money, 100% code coverage should be a minimum requirement. The current tests are lacking, as evidenced by bugs like issue 6.3, which should be caught by any end-to-end test that opens a loan and then uses a liquidation auction.
  • There are a number of opportunities for "griefing" in this system, where one participant can be forced to lock up their funds for a period of time or spend gas on transactions that won't benefit them. A reputation system may be necessary to protect participants from mischief makers.

5 Security Specification

This section describes, from a security perspective, the expected behavior of the system under audit. It is not a substitute for documentation. The purpose of this section is to identify specific security properties that were validated by the audit team.

5.1 Actors

The relevant actors are as follows:

  • Alice (borrower)
  • Bob (lender)
  • Carol (automated agent)
  • David (auction bidder)
  • Third parties who update oracles
  • Uninvolved third-party mischief makers

5.2 Trust Model

In any smart contract system, it's important to identify what trust is expected/required between various actors. For this audit, we established the following trust model:

  • Alice, Bob, and David do not need to trust each other.
  • No participant needs to trust Atomic Loans, the platform provider.
  • Alice and Bob must trust Carol, the automated agent. At first, this is likely to be the Atomic Loans platform itself, but in the future reputable third parties will also be used.
  • Collusion between any pair of Alice, Bob, and Dave should not disadvantage the remaining party.
  • No one needs to trust third parties who update oracles

5.3 Important Security Properties

The following is a non-exhaustive list of security properties that were verified in this audit:

  • No one should be able to influence a loan they are not party to.
  • Collateral should be refunded to the borrower if and only if a loan is canceled or successfully repaid.
  • Collateral should be seized by the lender if and only if the loan is not repaid, and even then only if liquidation sales were unsuccessful.
  • A borrower should not be able to receive a loan without locking the agreed upon collateral.
  • Third parties who update oracles should not be able to influence the prices those oracles obtain (only the timing of when they obtain those values).
  • Front running should never adversely impact any participant in a loan.

5.4 "Griefing"

Participants can cause each other temporary inconvenience or expense (known as "griefing") at various points during the process. Much of this is due to the cross-chain nature of the protocol. For example, Alice can request a loan from Bob via the Funds contract on the Ethereum blockchain, at which point a loan is opened and funds are transferred to the Loans contract. If Alice then never locks collateral on the Bitcoin blockchain, Bob can do nothing but cancel the loan and return the principal to the Funds contract (at his expense). If a fund is not used, Alice and Bob agree off chain to open a loan. One of them then opens that loan on the Ethereum blockchain, spending gas to do so. The other party can then simply leave without doing anything.

These and other griefing opportunities may be unavoidable until there is some way to prove on-chain that actions took place on the other chain (e.g. Bitcoin SPV proofs).

6 Issues

Each issue has an assigned severity:

  • Minor issues are subjective in nature. They are typically suggestions around best practices or readability. Code maintainers should use their own judgment as to whether to address such issues.
  • Medium issues are objective in nature but are not security vulnerabilities. These should be addressed unless there is a clear reason not to.
  • Major issues are security vulnerabilities that may not be directly exploitable or may require certain conditions in order to be exploited. All major issues should be addressed.
  • Critical issues are directly exploitable security vulnerabilities that need to be fixed.

The following table contains all the issues discovered during the audit, ordered based on their severity.

Chapter Issue Title Issue Status Severity
6.1 Reentrancy attack on Funds.pull can lead to draining funds Closed Critical
6.2 Oracles use testing contracts Closed Critical
6.3 Loans.setSechs off-by-one error leads to secret reuse Closed Critical
6.4 Partial payments are mishandled and can be lost Closed Critical
6.5 Funds.sechi is never updated, leading to reused secrets Closed Critical
6.6 Reentrancy attack on Loans.pull can lead to draining funds Closed Critical
6.7 Loans.unpay can be called repeatedly to drain tokens Closed Critical
6.8 Reentrancy attack on Sales.take can lead to draining funds Closed Critical
6.9 Bidders can prevent any higher bids from being accepted Closed Critical
6.10 Return values from ERC20 transfer, transferFrom, and approve calls must be checked Closed Critical
6.11 Sales.take transfers too little to the lender (and too much to the borrower) Closed Major
6.12 Lender and borrower can collude to prevent the high bidder from receiving a refund Closed Major
6.13 Loans.pay allows anyone to force the borrower to repay a loan Closed Major
6.14 Funds.gsech integer underflow Closed Medium
6.15 Funds contract initialization race condition Closed Medium
6.16 Anyone can frontrun Loans.pull() to modify the fund boolean Closed Medium
6.17 Oracle.pack race condition can lead to multiple reward payouts Closed Medium
6.18 Chainlink-based oracles use Ropsten job IDs Closed Medium
6.19 Oracle uses low-level call unnecessarily Closed Medium
6.20 Currency should not be a user-supplied contract Closed Medium
6.21 Vars should not be a user-supplied contract Closed Medium
6.22 Loan contract initialization race condition Closed Medium
6.23 Medianizer dependent on oracle order Closed Minor
6.24 Oracle inheritance model concerns Closed Minor
6.25 Reentrancy attack in Funds.push Closed Minor
6.26 Oracles: use a specific, recent Solidity version Closed Minor
6.27 Ownable in Chainlinked.sol/ChainlinkedTesting.sol is unused Closed Minor
6.28 Oracle.pack implementation is unused Closed Minor
6.29 Oracle.eval is unneeded Closed Minor
6.30 Oracle.push is unneeded Closed Minor
6.31 Oraclize has an unnecessary payable fallback function Closed Minor
6.32 Many variable names and function names obfuscate their meaning Closed Minor
6.33 Medianizer should use a more specific type rather than address Closed Minor
6.34 DSValue.setMax is unused Closed Minor
6.35 Medianizer should use bytes32 and uint256 rather than bytes12 and uint96 for efficiency Closed Minor
6.36 Averaging median values in Oraclize is unnecessarily complicated Closed Minor
6.37 Medianizer.values can be an array Closed Minor
6.38 Medianizer.indexes is unnecessary Closed Minor
6.39 Token approval in Medianizer is unnecessary Closed Minor
6.40 Consider replacing constant functions with pure or view in oracle code Closed Minor
6.41 loanExpiration should be renamed in the collateral swap provider Fix Not Reviewed Minor
6.42 Sales should expose an additional "swap expiration" Closed Minor
6.43 Bitcoin scripts can be merged into a single script Fix Not Reviewed Minor
6.44 Consider using a whitelist for allowed ERC20 tokens Closed Minor
6.45 BitcoinCollateralAgentProvider scripts accepts short secrets Fix Not Reviewed Minor
6.46 Loans.sechi reduces clarity and increases gas costs Closed Minor
6.47 Redundant struct definitions in Sales Closed Minor
6.48 Use specific contract types rather than address Closed Minor
6.49 Use external instead of public where possible Closed Minor
6.50 Vars functions should be view Closed Minor
6.51 Vars should be an interface Closed Minor

6.1 Reentrancy attack on Funds.pull can lead to draining funds

Severity Status Remediation Comment
Critical Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/19.

Description

Funds.pull transfers funds before updating the corresponding balance.

code/ethereum/contracts/Funds.sol:L201-L206

function pull(bytes32 fund, uint256 amt) public { // Pull funds from Loan Fund
    require(msg.sender == own(fund));
    require(bal(fund)  >= amt);
    funds[fund].tok.transfer(own(fund), amt);
    funds[fund].bal = sub(funds[fund].bal, amt);
}

If an ERC223 or ERC777 token (or another token that calls out to the sender or recipient during transfer) is used as the principal token, any fund owner can make a reentrant call to Funds.pull, causing funds to be transferred again. This can be done repeatedly until the Funds contract has been drained of that token type.

Remediation

The checks-effects-interactions pattern dictates that all state changes should happen before any calls are made to external contracts. Moving the balance update above the transfer would fix this particular reentrancy attack.

6.2 Oracles use testing contracts

Severity Status Remediation Comment
Critical Closed This will be fully resolved at the time of deployment, but it has been addressed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/22 by adding a comment with the correct production import.

Description

OraclizeAPI.sol vs. OraclizeAPITesting.sol and Chainlinked.sol vs. ChainlinkedTesting.sol

This needs to be fixed before deployment because the "testing" variety disable critical security checks.

Remediation

Switch to the production contracts before deployment.

6.3 Loans.setSechs off-by-one error leads to secret reuse

Severity Status Remediation Comment
Critical Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/20.

Description

This issue was discovered by the client during the initial audit phase.

code/ethereum/contracts/Loans.sol:L236-L241

sechs[loan].sechA1 = bsechs[0];
sechs[loan].sechAS = [ bsechs[0], bsechs[1], bsechs[2] ];
sechs[loan].sechB1 = lsechs[0];
sechs[loan].sechBS = [ lsechs[0], lsechs[1], lsechs[2] ];
sechs[loan].sechC1 = asechs[0];
sechs[loan].sechCS = [ asechs[0], asechs[1], asechs[2] ];

For each participant A, B, and C, four secret hashes are passed in, but the last one is unused and the first one is used twice. A reused secret is often catastrophic, as it means the secret is revealed before the appropriate time.

This bug can also go unnoticed by the participants, simply leading to them being unable to complete the loan process.

Remediation

Fix the off-by-one error:

sechs[loan].sechAS = [bsechs[1], bsechs[2], bsechs[3]];
...

6.4 Partial payments are mishandled and can be lost

Severity Status Remediation Comment
Critical Closed This issue is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/21.

Description

Before a sale is started, the borrower may have already paid back part of the loan. The following code transfers the amount that's already been paid back to the Sales contract:

code/ethereum/contracts/Loans.sol:L340

tokes[loan].transfer(address(sales), back(loan));

This happens every time a sale is started (each call to Loans.sell), so it happens up to three times. This means that even though 1x the amount is in the contract, 3x is transferred. This results in either failure or transferring tokens that rightfully belong to another loan.

This code from Sales.unpush transfers the amount to the borrower, but it does it only once (after the third sale):

code/ethereum/contracts/Sales.sol:L286-L288

if (next(sales[sale].loani) == 3) {
    tokes[sale].transfer(sales[sale].bor, loans.back(sales[sale].loani));
}

Remediation

Only transfer the partial payment to the Sales contract once.

6.5 Funds.sechi is never updated, leading to reused secrets

Severity Status Remediation Comment
Critical Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/22.

Description

It's important in HTLCs that secrets are not reused. If a previously revealed secret is used for a new HTLC, the beneficiary of that HTLC can immediately claim the funds (because the locking secret has already been revealed).

Funds.sechi determines which secret hashes are used to create new loans, but it is never updated, so it always has the value 0. This means the same four secrets are reused each time:

code/ethereum/contracts/Funds.sol:L229-L248

function lsech(                // Loan Set Secret Hashes
    bytes32 fund,              // Fund Index
    bytes32 loan,              // Loan Index
    bytes32[4] memory sechs_,  // 4 Secret Hashes
    bytes memory pubk_         // Public Key
) private { // Loan set Secret Hash and PubKey
    loans.setSechs(
        loan,
        sechs_,
        gsech(own(fund)),
        gsech(agent(fund)),
        pubk_,
        pubks[own(fund)]
    );
}

function gsech(address addr) private view returns (bytes32[4] memory) { // Get 4 secrethashes for loan
    require((sechs[addr].length - sechi[addr]) >= 4);
    return [ sechs[addr][add(sechi[addr], 0)], sechs[addr][add(sechi[addr], 1)], sechs[addr][add(sechi[addr], 2)], sechs[addr][add(sechi[addr], 3)] ];
}

Remediation

Add 4 to sechi every time a new loan is created.

6.6 Reentrancy attack on Loans.pull can lead to draining funds

Severity Status Remediation Comment
Critical Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/23.

Description

Loans.pull can't be called twice because it checks !off(loan) at the top and sets bools[loan].off = true at the bottom, but because it has no reentrancy protection, it can be called multiple times as part of the same transaction.

If an ERC223 or ERC777 token (or another token that calls out to the sender or recipient during transfer) is used as the auction token, any party receiving funds (the lender or agent) can make a reentrant call to pull, causing funds to be transferred again. This can be done repeatedly until the Loans contract has been drained of that token type.

code/ethereum/contracts/Loans.sol:L298-L315

function pull(bytes32 loan, bytes32 sec, bool fund) public { // Accept or Cancel // Bool fund set true if lender wants fund to return to fund
    require(!off(loan));
    require(bools[loan].taken == false || bools[loan].paid == true);
    require(sha256(abi.encodePacked(sec)) == sechs[loan].sechB1 || sha256(abi.encodePacked(sec)) == sechs[loan].sechC1);
    require(now                             <= acex(loan));
    require(bools[loan].sale                == false);
    if (bools[loan].taken == false) {
        tokes[loan].transfer(loans[loan].lend, loans[loan].prin);
    } else if (bools[loan].taken == true) {
        if (fundi[loan] == bytes32(0) || !fund) {
            tokes[loan].transfer(loans[loan].lend, lent(loan));
        } else {
            funds.push(fundi[loan], lent(loan));
        }
        tokes[loan].transfer(loans[loan].agent, lfee(loan));
    }
    bools[loan].off = true;
}

Remediation

The checks-effects-interactions pattern dictates that all state changes should happen before any calls are made to external contracts. Moving bools[loan].off = true to above all the transfers would fix this particular reentrancy attack.

A safer fix might be to use the withdrawal pattern everywhere. Track where funds are supposed to go and allow the various participants to withdraw them. This way transfers can be isolated to only one recipient at a time, and reentrancy can be solved locally in a straightforward way.

6.7 Loans.unpay can be called repeatedly to drain tokens

Severity Status Remediation Comment
Critical Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/25.

Description

Loans.unpay can be called repeatedly, and it will send tokens to the borrower each time. By calling this repeatedly, a borrower can drain the contract of that token. This is a big problem if there are other loans using the same token:

code/ethereum/contracts/Loans.sol:L285-L292

function unpay(bytes32 loan) public { // Refund payback
	require(!off(loan));
    require(!sale(loan));
	require(now              >  acex(loan));
	require(bools[loan].paid == true);
	require(msg.sender       == loans[loan].bor);
	tokes[loan].transfer(loans[loan].bor, owed(loan));
}

Remediation

Update the loan's state in some way that prevents unpay from being called again.

6.8 Reentrancy attack on Sales.take can lead to draining funds

Severity Status Remediation Comment
Critical Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/26.

Description

Sales.take can't be called twice because it checks !taken(sale) at the top and sets sales[sale].taken = true at the bottom, but because it has no reentrancy protection, it can be called multiple times as part of the same transaction.

If an ERC223 or ERC777 token (or another token that calls out to the sender or recipient during transfer) is used as the auction token, any party receiving funds (the lender, agent, or borrower) can make a reentrant call to take, causing funds to be transferred again. This can be done repeatedly until the Sales contract has been drained of that token type.

code/ethereum/contracts/Sales.sol:L300-L318

	function take(bytes32 sale) public { // Withdraw Bid (Accept Bid and disperse funds to rightful parties)
        require(!taken(sale));
		require(now > sales[sale].salex);
		require(hasSecs(sale));
		require(sha256(abi.encodePacked(sechs[sale].secD)) == sechs[sale].sechD);

        if (sales[sale].bid > (loans.dedu(sales[sale].loani))) {
            tokes[sale].transfer(sales[sale].lend, loans.lentb(sales[sale].loani));
            if (agent(sale) != address(0)) {
                tokes[sale].transfer(sales[sale].agent, loans.lfee(sales[sale].loani));
            }
            tokes[sale].approve(address(med), loans.lpen(sales[sale].loani));
            med.push(loans.lpen(sales[sale].loani), tokes[sale]);
            tokes[sale].transfer(sales[sale].bor, add(sub(sales[sale].bid, loans.dedub(sales[sale].loani)), loans.back(sales[sale].loani)));
        } else {
            tokes[sale].transfer(sales[sale].lend, sales[sale].bid);
        }
        sales[sale].taken = true;
	}

Remediation

The checks-effects-interactions pattern dictates that all state changes should happen before any calls are made to external contracts. Moving sales[sale].taken = true to above all the transfers would fix this particular reentrancy attack.

A safer fix might be to use the withdrawal pattern everywhere. Track where funds are supposed to go and allow the various participants to withdraw them. This way transfers can be isolated to only one recipient at a time, and reentrancy can be solved locally in a straightforward way.

6.9 Bidders can prevent any higher bids from being accepted

Severity Status Remediation Comment
Critical Closed This is addressed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/44. Now only a single token is allowed per contract, and that token can be chosen carefully to avoid problematic implementations.

Description

In the bid function Sales.push, the current highest bidder's bid is returned to them by calling transfer on the auction token. If the current high bidder can block this transfer from succeeding, they can stop all new bids from being accepted. This allows them to win the auction with an arbitrarily low bid.

Some token standards, such as ERC223 and ERC777 call out to transfer recipient contracts to see if they want to allow the transfer. An attacker could participate as a contract and refuse transfers until after the auction has ended.

code/ethereum/contracts/Sales.sol:L229-L252

    function push(     // Bid on Collateral
    	bytes32 sale,  // Auction Index
    	uint256 amt,   // Bid Amount
    	bytes32 sech,  // Secret Hash
    	bytes20 pbkh   // PubKeyHash
	) public {
        require(msg.sender != bor(sale) && msg.sender != lend(sale));
		require(sales[sale].set);
    	require(now < sales[sale].salex);
    	require(amt > sales[sale].bid);
    	require(tokes[sale].balanceOf(msg.sender) >= amt);
    	if (sales[sale].bid > 0) {
    		require(amt > rmul(sales[sale].bid, vares[sale].MINBI())); // Make sure next bid is at least 0.5% more than the last bid
    	}

    	tokes[sale].transferFrom(msg.sender, address(this), amt);
    	if (sales[sale].bid > 0) {
    		tokes[sale].transfer(sales[sale].bidr, sales[sale].bid);
    	}
    	sales[sale].bidr = msg.sender;
    	sales[sale].bid  = amt;
    	sechs[sale].sechD = sech;
    	sales[sale].pbkh = pbkh;
	}

Remediation

Use the "withdrawal pattern" instead. Instead of immediately transferring the tokens to the previous high bidder, keep track of what refunds are owed and supply a function for bidders to call to collect their refunds.

6.10 Return values from ERC20 transfer, transferFrom, and approve calls must be checked

Severity Status Remediation Comment
Critical Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/18 and https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/8.

Description

ERC20's transfer, transferFrom, and approve functions return a boolean indicating success. Many implementations revert failed transfers, but certainly not all do. Because of this, the return value must be checked. Otherwise it may seem that the call succeeded when it actually failed.

Examples

In Loans.push, a failed transferFrom could mean that the loan appears to be funded when it is actually not. An attacker could use this to steal that ERC20 token from other users. The attacker would create a loan, supply insufficient funds such that the transfer failed, and then cancel the loan. This would cause the Loans contract to send any tokens it was holding to the attacker:

code/ethereum/contracts/Loans.sol:L247-L251

	function push(bytes32 loan) public { // Fund Loan
		require(sechs[loan].set);
    	require(bools[loan].pushed == false);
    	tokes[loan].transferFrom(msg.sender, address(this), prin(loan));
    	bools[loan].pushed = true;

Similarly, in Loans.pay, an attacker could convince the contract that they have paid back a loan when they haven't. Again, this could be used by an attacker to steal tokens from the Loans contract, which would believe that the principal has been repaid and can safely be transferred to the lender:

code/ethereum/contracts/Loans.sol:L270-L278

function pay(bytes32 loan, uint256 amt) public { // Payback Loan
    // require(msg.sender                == loans[loan].bor); // NOTE: this is not necessary. Anyone can pay off the loan
	require(!off(loan));
    require(!sale(loan));
	require(bools[loan].taken         == true);
	require(now                       <= loans[loan].loex);
	require(add(amt, backs[loan])     <= owed(loan));

	tokes[loan].transferFrom(loans[loan].bor, address(this), amt);

In Sales.push, an attacker could make a bid without transferring any funds. An attacker could put in a fake bid and either win the auction for free or wait for a higher bidder to "refund" tokens to the attacker that had never been paid in the first place:

code/ethereum/contracts/Sales.sol:L229-L244

    function push(     // Bid on Collateral
    	bytes32 sale,  // Auction Index
    	uint256 amt,   // Bid Amount
    	bytes32 sech,  // Secret Hash
    	bytes20 pbkh   // PubKeyHash
	) public {
        require(msg.sender != bor(sale) && msg.sender != lend(sale));
		require(sales[sale].set);
    	require(now < sales[sale].salex);
    	require(amt > sales[sale].bid);
    	require(tokes[sale].balanceOf(msg.sender) >= amt);
    	if (sales[sale].bid > 0) {
    		require(amt > rmul(sales[sale].bid, vares[sale].MINBI())); // Make sure next bid is at least 0.5% more than the last bid
    	}

    	tokes[sale].transferFrom(msg.sender, address(this), amt);

Other examples include:

code/ethereum/contracts/Sales.sol:L269

tokes[sale].approve(address(med), loans.lpen(sales[sale].loani));

code/ethereum/contracts/Funds.sol:L136

tok_.approve(address(loans), 2**256-1);

code/ethereum/contracts/Loans.sol:L221

tok_.approve(address(funds), 2**256-1);

code/oracles/contracts/Medianizer.sol:L72

tok.approve(values[bytes12(j)], 2**256-1);

code/oracles/contracts/Medianizer.sol:L77

tok.transferFrom(msg.sender, values[bytes12(i)], uint(div(uint128(amt), uint128(next) - 1)));

code/oracles/contracts/Oracle.sol:L46

tok_.transferFrom(msg.sender, address(this), uint256(amt));

code/oracles/contracts/Oracle.sol:L101

tok.transfer(owed, gain);

code/oracles/contracts/chainlink/ChainLink.sol:L24

link.transferFrom(msg.sender, address(this), uint(pmt_));

code/oracles/contracts/chainlink/ChainLink.sol:L61

tok.transfer(owed, min(maxr, gain));

code/oracles/contracts/oraclize/Oraclize.sol:L34

weth.transferFrom(msg.sender, address(this), uint(pmt_));

Remediation

Wrap all calls to transfer, transferFrom, and approve in a require statement to ensure transactions are reverted when an ERC20 transfer fails.

6.11 Sales.take transfers too little to the lender (and too much to the borrower)

Severity Status Remediation Comment
Major Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/27.

Description

Sales.take is used by the borrower and lender to disperse the high bid in the auction. The following line transfers an amount to the lender:

code/ethereum/contracts/Sales.sol:L265

tokes[sale].transfer(sales[sale].lend, loans.lentb(sales[sale].loani));

The amount transferred comes from loans.lentb:

code/ethereum/contracts/Loans.sol:L124-L126

function lentb(bytes32 loan)  public view returns (uint256) { // Amount lent by lender minus amount paid back
    return sub(lent(loan), back(loan));
}

code/ethereum/contracts/Loans.sol:L120-L122

function lent(bytes32 loan)   public view returns (uint256) { // Amount lent by Lender
    return add(prin(loan), lint(loan));
}

The lender gets their initial principal plus interest minus the amount already the borrower already paid back, but the lender should be made whole by getting the full principal plus interest amount.

Because the borrower receives the remainder of the high bid plus the amount paid back, they are being overpaid.

Remediation

Transfer lent(...) to the lender, rather than lentb(...). Also make the corresponding change to the amount transferred to the borrower.

6.12 Lender and borrower can collude to prevent the high bidder from receiving a refund

Severity Status Remediation Comment
Major Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/28.

Description

In Sales.unpush, the high bidder can only receive a refund if the borrower and lender have not revealed their secrets:

code/ethereum/contracts/Sales.sol:L278-L282

	function unpush(bytes32 sale) public { // Refund Bid
        require(!taken(sale));
        require(!off(sale));
		require(now > sales[sale].setex);
		require(!hasSecs(sale));

This means that the lender and borrower can lock the high bidder's funds forever simply by revealing those secrets. This opens up a ransom opportunity:

  • Alice and Bob (probably the same person), acting as borrower and lender, are conspiring to steal from David.
  • They establish a loan and default.
  • David is the high bidder in the liquidation option.
  • When the sales expiration is reached, Alice and Bob reveal their secrets A and B to the Sales contract, but they do not move collateral to the collateral swap provider.
  • David has no way to retrieve the collateral, and he has no way to receive a refund.
  • Alice and Bob still have the ability to complete the sale, but they can demand that David pay a ransom first.

Remediation

The high bidder should always be able to receive a refund if the settlement expiration has passed without the borrower and lender completing the sale.

6.13 Loans.pay allows anyone to force the borrower to repay a loan

Severity Status Remediation Comment
Major Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/29.

Description

Loans.pay can be called by anyone. Regardless of who calls it, it then attempts a transferFrom from the borrower. This means that anyone can force the borrower to repay a loan up to the amount they've approved to the Loan contract. This is especially exploitable in a front-running situation where the borrower approves the Loan contract because they are trying to repay one loan, but someone submits a transaction to repay a different loan (with the same borrower) instead.

code/ethereum/contracts/Loans.sol:L270-L278

function pay(bytes32 loan, uint256 amt) public { // Payback Loan
    // require(msg.sender                == loans[loan].bor); // NOTE: this is not necessary. Anyone can pay off the loan
	require(!off(loan));
    require(!sale(loan));
	require(bools[loan].taken         == true);
	require(now                       <= loans[loan].loex);
	require(add(amt, backs[loan])     <= owed(loan));

	tokes[loan].transferFrom(loans[loan].bor, address(this), amt);

Remediation

Either require the caller to be the borrower or transfer funds from msg.sender instead of always transferring from the borrower.

6.14 Funds.gsech integer underflow

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/22.

Description

code/ethereum/contracts/Funds.sol:L245-L248

function gsech(address addr) private view returns (bytes32[4] memory) { // Get 4 secrethashes for loan
    require((sechs[addr].length - sechi[addr]) >= 4);
    return [ sechs[addr][add(sechi[addr], 0)], sechs[addr][add(sechi[addr], 1)], sechs[addr][add(sechi[addr], 2)], sechs[addr][add(sechi[addr], 3)] ];
}

The require statement suffers from an integer underflow. For example, consider when the length of sechs[addr] is 3 and sechi[addr] is 4. With underflow, 3 - 4 >= 4, so the require statement will pass even though there aren't enough secrets left in the array.

Fortunately, there's no consequence to this underflow because the array access will be bounds checked, and attempts to read past the end of the array will fail anyway.

Remediation

Either remove the check because the array bounds check is already taking care of it, or rewrite the check to not suffer from the integer underflow.

6.15 Funds contract initialization race condition

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/30.

Description

The Funds contract contains the state variable on and the function setLoans to ensure that the loans state variable is initialized at most once, but there's no mechanism to ensure it's only set by the contract owner. This opens up a race condition in which an attacker can set the loans state variable to any contract they want. If that were to go unnoticed, it would allow an attacker to manipulate or block all future auctions.

The code would be simpler and safer if the Loans contract were simply deployed in the Funds constructor or passed in as a parameter. The following code could then be removed:

code/ethereum/contracts/Funds.sol:L24

bool on; // Ensure that Loans contract is created

code/ethereum/contracts/Funds.sol:L43-L47

function setLoans(address loans_) public {
    require(!on);
    loans = Loans(loans_);
    on = true;
}

Remediation

Deploy the Loans contract in the Funds constructor or pass the Loans contract into the Funds constructor.

6.16 Anyone can frontrun Loans.pull() to modify the fund boolean

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/31.

Description

When Bob calls Loans.pull() at the end of the loan period, he does so by revealing B1. This gives anyone an opportunity to front run the transaction and modify parameters being sent in, specifically the fund boolean that determines how the loan principal and interest are delivered.

code/ethereum/contracts/Loans.sol:L298-L315

function pull(bytes32 loan, bytes32 sec, bool fund) public { // Accept or Cancel // Bool fund set true if lender wants fund to return to fund
    require(!off(loan));
    require(bools[loan].taken == false || bools[loan].paid == true);
    require(sha256(abi.encodePacked(sec)) == sechs[loan].sechB1 || sha256(abi.encodePacked(sec)) == sechs[loan].sechC1);
    require(now                             <= acex(loan));
    require(bools[loan].sale                == false);
    if (bools[loan].taken == false) {
        tokes[loan].transfer(loans[loan].lend, loans[loan].prin);
    } else if (bools[loan].taken == true) {
        if (fundi[loan] == bytes32(0) || !fund) {
            tokes[loan].transfer(loans[loan].lend, lent(loan));
        } else {
            funds.push(fundi[loan], lent(loan));
        }
        tokes[loan].transfer(loans[loan].agent, lfee(loan));
    }
    bools[loan].off = true;
}

The impact of this bug is minimal because at worst, the lender needs to just transfer the funds to the correct location.

Remediation

Either remove the fund parameter altogether and assume lenders always want funds returned to the Fund contract, or require that only Bob (lender) or Carol (automated agent) can call Loans.pull.

6.17 Oracle.pack race condition can lead to multiple reward payouts

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/11.

Description

The oracles currently use "global" state variables to keep track of the last person to call pack successfully, how much they paid, and what token they would like to be rewarded in. There's a potential race condition here:

  1. Alice calls pack, which triggers an asynchronous request to Oraclize for data.
  2. Bob calls pack.
  3. Oraclize calls __callback for Alice's request, but Bob is paid the reward.
  4. Oraclize cals __callback for Bob's request, and Bob is paid again.

This risk is mitigated by the use of a cool down period. The lag state variable is used to keep track of the next valid time a call to pack can be made, and it is intended to be sufficiently long that a request to Oraclize or Chainlink will complete before a subsequent request can be made.

However, no cool down period is sufficiently long to handle failures that may occur (e.g. Oraclize having downtime).

A better approach might be to keep track of this data per request and reward accordingly no matter when the results arrive. Doing this also makes it easy to ensure the same request isn't fulfilled twice.

Remediation

Use a mapping of request IDs to the relevant fields:

struct AsyncRequest {
    address sender;
    uint256 payment;
    ERC20 rewardToken;
    bool valueReceived; // "posted"
    bool paymentCurrencyPriceReceived; // "told"
}

mapping(bytes32 => AsyncRequest) asyncRequests;

Store the values when pack is called, and use them when the requests are completed to pay the appropriate reward and check for duplicate callbacks.

6.18 Chainlink-based oracles use Ropsten job IDs

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/25.

Description

This constant and other Chainlink job IDs are for the Ropsten test network:

code/oracles/contracts/chainlink/CoinMarketCap.sol:L7

bytes32 constant UINT256_MUL_JOB = bytes32("ce36a79ea04c4d3ca015d267784417bd");

comes from https://docs.chain.link/docs/coinmarketcap, should be from https://docs.chain.link/docs/coinmarketcap-chainlink-ethereum-mainnet.

Remediation

Be sure to switch to the main network job IDs before deployment.

6.19 Oracle uses low-level call unnecessarily

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/10.

Description

code/oracles/contracts/Oracle.sol:L87

med.call(abi.encodeWithSignature("poke()"));

Not only does this code unnecessarily bypass Solidity's type checking, but it also ignores any failures that might occur in poke().

Remediation

Use med.poke() instead, which is simpler and propagates failures. If there's a reason to ignore reverts in the med contract, add a comment explaining why.

6.20 Currency should not be a user-supplied contract

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/32.

Description

The Currency contract is currently supplied by the user who opens the loan. This contract is responsible for, among other things, performing currency-specific multiplication to determine the current value of a loan's collateral. If a malicious contract is used, the contract owner could manipulate this value to make a loan subject to liquidation when it shouldn't be.

Mitigating factors

Auctions have no effect unless both parties agree to their outcome, so manipulating when an auction can start is not a big threat. Similarly, preventing an auction from starting isn't very interesting, given that either party can refuse its outcome anyway.

Remediation

Get rid of the Currency contract altogether. Instead of using the number of decimal places used by the collateral currency, use a hardcoded multiplier to understand results returned from the medianizer.

6.21 Vars should not be a user-supplied contract

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/33.

Description

The Vars contract is currently supplied by the user who opens the loan. This makes it very difficult for other parties to verify the terms of the loan because those terms may change if the contract returns different values in the future. To be safe, all parties must audit the code for the supplied Vars contract and verify its behavior.

A malicious Vars contract can also do things like help a particular bidder win an auction. When called by the Sales.push function to retrieve the minimum bid increment, a malicious Vars contract can initially return a reasonable number but later return a very high number (or simply revert) to block new bidders from competing:

code/ethereum/contracts/Sales.sol:L186-L199

    function push(     // Bid on Collateral
    	bytes32 sale,  // Auction Index
    	uint256 amt,   // Bid Amount
    	bytes32 sech,  // Secret Hash
    	bytes20 pbkh   // PubKeyHash
	) public {
        require(msg.sender != bor(sale) && msg.sender != lend(sale));
		require(sales[sale].set);
    	require(now < sales[sale].salex);
    	require(amt > sales[sale].bid);
    	require(tokes[sale].balanceOf(msg.sender) >= amt);
    	if (sales[sale].bid > 0) {
    		require(amt > rmul(sales[sale].bid, vares[sale].MINBI())); // Make sure next bid is at least 0.5% more than the last bid
    	}

Remediation

A few suggestions for how to address this, in order of descending preference:

  1. Get rid of Vars altogether and store the values directly in state variables. These values then need to be passed around from time to time between contracts (e.g. in Funds.lopen and Loans.sell).
  2. Only allow one, audited Vars implementation. This single implementation could be deployed as needed with the parameters for a given loan. This way the code need only be verified once.
  3. Use a multitenant Vars contract that stores per-loan values in a mapping, and do all lookups by querying this singleton contract.

6.22 Loan contract initialization race condition

Severity Status Remediation Comment
Medium Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/34.

Description

The Loans contract contains the state variable on and the function setSales to ensure that the sales state variable is initialized at most once, but there's no mechanism to ensure it's only set by the contract owner. This opens up a race condition in which an attacker can set the sales state variable to any contract they want. If that were to go unnoticed, it would allow an attacker to manipulate or block all future auctions.

The code would be simpler and safer if the Sales contract were simply created in the Loan constructor. The following code could then be removed:

code/ethereum/contracts/Loans.sol:L31

bool on; // Ensure that Sales contract is created

code/ethereum/contracts/Loans.sol:L186-L190

function setSales(address sales_) public {
    require(!on);
    sales = Sales(sales_);
    on = true;
}

Remediation

Add sales = new Sales(this, med); to the Loans constructor and get rid of on and setSales.

6.23 Medianizer dependent on oracle order

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/9.

Description

Medianizer.setMax calls setMax on only the first five oracles:

code/oracles/contracts/Medianizer.sol:L58-L66

function setMax(uint256 maxr_) {
	require(on);
	require(msg.sender == own);
	DSValue(values[bytes12(1)]).setMax(maxr_);
	DSValue(values[bytes12(2)]).setMax(maxr_);
	DSValue(values[bytes12(3)]).setMax(maxr_);
	DSValue(values[bytes12(4)]).setMax(maxr_);
	DSValue(values[bytes12(5)]).setMax(maxr_);
}

This means it's necessary to include the Chainlink-based oracles at those first five positions in the array. Otherwise setMax won't be called where it should be, or it will be called on an oracle that doesn't actually have that functionality.

Remediation

Consider implementing setMax at the base Oracle layer and having it be a no-op there. Then it could safely be called on the entire set of oracles without issue.

6.24 Oracle inheritance model concerns

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/9.

Description

The inheritance model used by the oracle contracts makes the code hard to verify:

  1. setMax is in DSValue, but the only thing that inherits from DSValue is Medianizer, and it overloads that. This is good because the implementation there is bizarre (just sets has to true).
  2. The Medianizer casts Oracles to DSValue even though they don’t inherit from that. This is to call setMax, which shouldn’t be in DSValue anyway, and peek, which could just be used from Oracle (the proper type).
  3. Oracle has an implementation of pack that is never used (overloaded everywhere).
  4. Oracle has medm and an implementation of chec that uses it even though these only apply to contracts that derive from Oraclize (so that code should go there).
  5. Oracle has a harmful implementation of call (one that extends the expiration of the existing value), but fortunately it's overridden in every actual oracle implementation.

Remediation

Make sure functionality is introduced at the right layer of the inheritance and that all casts are done to appropriate types.

6.25 Reentrancy attack in Funds.push

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/35.

Description

Similar to issue 6.1, Funds.push performs a transfer before the corresponding balance update:

code/ethereum/contracts/Funds.sol:L141-L145

function push(bytes32 fund, uint256 amt) public { // Push funds to Loan Fund
    // require(msg.sender == own(fund) || msg.sender == address(loans)); // NOTE: this require is not necessary. Anyone can fund someone elses loan fund
    funds[fund].tok.transferFrom(msg.sender, address(this), amt);
    funds[fund].bal = add(funds[fund].bal, amt);
}

This is not obviously exploitable, but it's worth addressing.

Remediation

Move the balance update to before the transfer.

6.26 Oracles: use a specific, recent Solidity version

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/13.

Description

Choose a specific version of Solidity and make sure all pragmas are for that specific version. Preferably, choose the current Solidity version 0.5.10. If it's impossible to upgrade to 0.5.x, choose the most recent 0.4.x version, 0.4.26. In general, newer compiler versions have fewer bugs than older ones.

Examples

code/oracles/contracts/Medianizer.sol:L4

pragma solidity ^0.4.8;

code/oracles/contracts/Oracle.sol:L1

pragma solidity >0.4.18;

Remediation

Use specific pragma directives in code to pin the code to a particular (recent) version of Solidity.

6.27 Ownable in Chainlinked.sol/ChainlinkedTesting.sol is unused

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/14.

Description

Chainlinked.sol came from https://gist.github.com/thodges-gh/24c1ef93cb91458e936ff4e53cbb59b0, which includes Ownable because it's used in the same code. It's not used by any contract in the oracle system.

Remediation

Remove the unused Ownable contract.

6.28 Oracle.pack implementation is unused

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/9.

Description

Both Oraclize and ChainLink overload the implementation of pack and do not call the base implementation.

Remediation

Remove the dead code by deleting the Oracle.pack implementation.

6.29 Oracle.eval is unneeded

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/15.

Description

code/oracles/contracts/Oracle.sol:L39-L43

function eval() public view
    returns (bytes32)
{
    return bytes32(uint(lval));
}

This function just casts lval to a bytes32 and returns it. It could be replaced by just making lval public. This would have the added benefit of returning a number instead of a bytes32.

Remediation

Remove eval and mark lval as public instead.

6.30 Oracle.push is unneeded

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/16.

Description

code/oracles/contracts/Oracle.sol:L45-L47

function push(uint128 amt, ERC20 tok_) public {
    tok_.transferFrom(msg.sender, address(this), uint256(amt));
}

This function just calls transferFrom on the specified token to transfer the specified amount. The caller could instead just call transfer directly and avoid having to approve tokens first. This is, in fact, what the medianizer does.

Remediation

Remove the unnecessary and unused push function.

6.31 Oraclize has an unnecessary payable fallback function

Severity Status Remediation Comment
Minor Closed This issue is invalid. The fallback function is needed to use the WETH withdraw function to convert WETH to ether.

Description

code/oracles/contracts/oraclize/Oraclize.sol:L21

function () public payable { }

This contract doesn't need to carry an ether balance, and allowing inbound ether transfers only leads to locked funds.

Remediation

Remove the fallback function.

6.32 Many variable names and function names obfuscate their meaning

Severity Status Remediation Comment
Minor Closed This is addressed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/50 and https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/26.

Description

Throughout the contract system, many cryptic names have been chosen that make it difficult to understand the code. Giving variables and functions descriptive names will make it easier for readers to have confidence that the code is doing what they expect.

Some of the worst offenders are listed below, but this is far from a complete list. As a good rule of thumb, if a comment or glossary is necessary to explain the name, it's probably better to use a more descriptive name.

Examples

code/oracles/contracts/Oracle.sol:L15

uint32 public zzz;

code/oracles/contracts/Oracle.sol:L98

function ward() internal { // Reward

code/oracles/contracts/Medianizer.sol:L90

bytes32[] memory wuts = new bytes32[](uint96(next) - 1);

code/oracles/contracts/Medianizer.sol:L94

var (wut, wuz) = DSValue(values[bytes12(i)]).peek();

code/ethereum/contracts/Loans.sol:L35

address lend;       // Address Lender

code/ethereum/contracts/Loans.sol:L37

uint256 born;       // Created At

code/ethereum/contracts/Loans.sol:L38

uint256 loex;       // Loan Expiration

code/ethereum/contracts/Loans.sol:L40

uint256 lint;       // Interest

code/ethereum/contracts/Loans.sol:L44

uint256 rat;        // Liquidation Ratio

code/ethereum/contracts/Loans.sol:L61

bool marked;        // Collateral Marked as Locked

code/ethereum/contracts/Loans.sol:L31

bool on; // Ensure that Sales contract is created

code/ethereum/contracts/Loans.sol:L80

function apex(bytes32 loan)   public returns (uint256) { // Approval Expiration

code/ethereum/contracts/Loans.sol:L84

function acex(bytes32 loan)   public returns (uint256) { // Acceptance Expiration

code/ethereum/contracts/Loans.sol:L88

function biex(bytes32 loan)   public returns (uint256) { // Bidding Expiration

code/ethereum/contracts/Loans.sol:L124

function lentb(bytes32 loan)  public view returns (uint256) { // Amount lent by lender minus amount paid back

code/ethereum/contracts/Loans.sol:L140

function dedub(bytes32 loan)  public view returns (uint256) { // Deductible amount from collateral minus amount paid back

code/ethereum/contracts/Loans.sol:L168

function colv(bytes32 loan) public returns (uint256) { // Current Collateral Value

code/ethereum/contracts/Sales.sol:L38

bool       set;    // Sale at index opened

code/ethereum/contracts/Sales.sol:L186

function push(     // Bid on Collateral

code/ethereum/contracts/Sales.sol:L240

function sec(bytes32 sale, bytes32 sec_) public { // Provide Secret

code/ethereum/contracts/Sales.sol:L278

function unpush(bytes32 sale) public { // Refund Bid

code/oracles/contracts/Oracle.sol:L9

uint128 constant public turn = 1010000000000000000; // minimum price change 1.01 (1%)

code/oracles/contracts/Oracle.sol:L21

uint128 dis;

code/oracles/contracts/Oracle.sol:L22

uint256 gain;

code/oracles/contracts/Oracle.sol:L39

function eval() public view

code/oracles/contracts/Oracle.sol:L57

function pack(uint128 pmt_, ERC20 tok_) { // payment

code/oracles/contracts/Oracle.sol:L76

function chec()

Remediation

Use more reader-friendly names that describe the purpose and functionality of the variable or function.

6.33 Medianizer should use a more specific type rather than address

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/20.

Description

Medianizer uses addresses in many places and then casts those addresses to DSValues. It would be better to just work with DSValues in the first place.

Even better would be to use a more accurate type, as the "values" are actually contracts that derive from Oracle.

Remediation

Use Oracle[] public oracles instead of mapping(bytes12 => address) values and don't attempt to cast those values into DSValues.

6.34 DSValue.setMax is unused

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/9.

Description

DSValue contains a strange implementation of setMax, which has no effect except to set has to true. This either has no effect or causes a 0 value to be incorrectly interpreted as valid:

code/oracles/contracts/DSValue.sol:L24-L26

function setMax(uint256 maxr) public {
    has = true;
}

This implementation of setMax seems to never actually be used, as only the Chainlink oracles have their setMax function called, and that's a different implementation (in a contract that doesn't inherit from DSValue in the first place).

Remediation

Remove the unused function and instead declare one in Oracle so the Medianizer can call it.

6.35 Medianizer should use bytes32 and uint256 rather than bytes12 and uint96 for efficiency

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/20.

Description

A number of variables in Medianizer are bytes12 or uint96, and sometimes values are cast from one to the other. Both are less efficient than their 256-bit counterparts: bytes32 and uint256 due to the overhead of masking the unused bits on storage and retrieval.

Remediation

Use bytes32 and uint256 everywhere. Because the values are actually numbers, the preference is to use uint256 exclusively.

6.36 Averaging median values in Oraclize is unnecessarily complicated

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/20.

Description

When an even number of values are present, the median is considered to be the average of the two elements closest to the center:

code/oracles/contracts/Medianizer.sol:L117-L119

uint128 val1 = uint128(wuts[(ctr / 2) - 1]);
uint128 val2 = uint128(wuts[ctr / 2]);
value = bytes32(wdiv(hadd(val1, val2), 2 ether));

This calculation is unnecessarily complex. All that's needed is to add the two numbers and divide by two.

Remediation

Simply use (val1 + val2) / 2. The values have already been cast to uint128s, which guarantees there is no overflow.

6.37 Medianizer.values can be an array

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/20.

Description

code/oracles/contracts/Medianizer.sol:L8-L10

mapping (bytes12 => address) public values;
mapping (address => bytes12) public indexes;
bytes12 public next = 0x1;

Instead of a mapping and a next pointer, values can simply be an array (with values.length serving the function of next).

Remediation

Use address[] public values (or better yet Oracle[] public values) instead and use values.length where it's needed.

6.38 Medianizer.indexes is unnecessary

Severity Status Remediation Comment
Minor Closed Fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/20.

Description

code/oracles/contracts/Medianizer.sol:L9

mapping (address => bytes12) public indexes;

The values in Medianizer.indexes are initialized in the set function and then never used again. It presumably comes from a version of the code that allowed changes to the list of oracles.

This code can now be deleted.

Remediation

Remove the unused code: indexes and everything in the set function that modifies that mapping.

6.39 Token approval in Medianizer is unnecessary

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/17.

Description

In the Medianizer contract, the tokens used for rewards are approved to be spent by each oracle, but it appears that no oracle actually takes advantage of this.

code/oracles/contracts/Medianizer.sol:L7

mapping (address => bool)    public tokas;  // Is ERC20 Token Approved

code/oracles/contracts/Medianizer.sol:L69-L75

    	if (tokas[address(tok)] == false) {
            tokas[address(tok)] = true;
            for (uint96 j = 1; j < uint96(next); j++) {
	    		tok.approve(values[bytes12(j)], 2**256-1);
	    	}
	    	tokas[address(tok)] = true;
        }

Remediation

Remove the dead code.

6.40 Consider replacing constant functions with pure or view in oracle code

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-oracle-contracts/pull/18.

Description

Declaring a function as constant is deprecated, and should be replaced with view or, if state is not being read from, with pure. DSMath.sol is the largest offender, but constant functions are also being used in DSValue.sol and Medianizer.sol. These updates will also make it easier to port the code to Solidity 0.5.x, which has built-in security benefits.

Examples

code/oracles/contracts/DSMath.sol:L9-L11

function add(uint256 x, uint256 y) constant internal returns (uint256 z) {
    assert((z = x + y) >= x);
}

Remediation

Most of these functions can be marked pure, but in instances where state is being read from, view would be more appropriate. Notably, this issue only exists in the oracle code, and the DSMath.sol from the loans contracts has already been updated appropriately.

6.41 loanExpiration should be renamed in the collateral swap provider

Severity Status Remediation Comment
Minor Fix Not Reviewed This is addressed in https://github.com/AtomicLoans/chainabstractionlayer-loans/pull/8, which has not been reviewed by the audit team. (It will be included in an upcoming audit.)

Description

The parameter loanExpiration is misnamed. It's not actually the loan expiration, but rather the last time an auction winner (David) can claim the collateral he won in the auction.

The parameter is declared here:

code/bitcoin/packages/bitcoin-collateral-swap-provider/lib/BitcoinCollateralSwapProvider.js:L27

createRefundableScript (borrowerPubKey, lenderPubKey, agentPubKey, bidderPubKeyHash, secretHashD1, loanExpiration, biddingExpiration) {

and used here:

code/bitcoin/packages/bitcoin-collateral-swap-provider/lib/BitcoinCollateralSwapProvider.js:L58-L67

loanExpirationPushDataOpcode, // OP_PUSHDATA({loanExpirationHexLength})
loanExpirationHexEncoded, // {loanExpirationHexEncoded}
'b1', // OP_CHECKLOCKTIMEVERIFY
'75', // OP_DROP
'52', // PUSH #2
borrowerPubKeyPushDataOpcode, borrowerPubKey, // OP_PUSHDATA({alicePubKeyLength}) {alicePubKey}
lenderPubKeyPushDataOpcode, lenderPubKey, // OP_PUSHDATA({bobPubKeyLength}) {bobPubKey}
agentPubKeyPushDataOpcode, agentPubKey, // OP_PUSHDATA({agentPubKeyLength}) {agentPubKey}
'53', // PUSH #3
'ae', // CHECKMULTISIG

Remediation

Rename the parameter to something like bidderSettlementExpiration or bidderClaimExpiration to avoid confusion.

6.42 Sales should expose an additional "swap expiration"

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/47.

Description

Sales currently has one expiration, setex ("settlement expiration"), which is used in two ways:

  1. The collateral swap script uses something like setex - 2 hours (currently planned to be hardcoded) as the first time the borrower (Alice) and lender (Bob) can move funds back to the refundable/seizable collateral scripts.
  2. The high bidder can only claim a refund (via unpush) after this timestamp has passed.

It's critical that there is sufficient time between (1) and (2) for Alice and Bob to react to the high bidder (David) spending the collateral.

Because the settlement period is configurable per loan, hardcoding a gap between (1) and (2) doesn't always make sense. For example, during a 48-hour settlement period, allowing just 2 hours at the end for Alice and Bob to call Sales.take before David can get a refund seems too short.

A good rule is to use half the settlement period for David to claim the collateral and the other half for Alice and Bob to react and claim their payment. To make this easy and clear, it would be nice if the Sales contract computed this value automatically and exposed it.

Remediation

Something like this would work:

sales[sale].setex = now + vars.SALEX() + vars.SETEX();     // existing setex
sales[sale].swapex = now + vars.SALEX() + vars.SETX() / 2; // added swapex

6.43 Bitcoin scripts can be merged into a single script

Severity Status Remediation Comment
Minor Fix Not Reviewed This is addressed in https://github.com/AtomicLoans/chainabstractionlayer-loans/pull/8, which has not been reviewed by the audit team. (It will be included in an upcoming audit.)

Description

The various Bitcoin script providers implement very similar scripts with much identical code. They could be merged into a single script with different parameters to reduce the surface area. Specifically, the seizable collateral agent script (created in BitoinCollateralAgentProvider.createSeizableScript) is a superset of all the other scripts.

Remediation

Use the combined script everywhere, with different parameters for the different use cases.

6.44 Consider using a whitelist for allowed ERC20 tokens

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/44. Contracts now operate with a single token, and new contracts must be deployed to support new tokens.

Description

Currently, any ERC20 token can be used as principal, but the system makes the assumption that only stablecoins are used. Supporting only a small set of whitelisted tokens would help signal to users that only stablecoins should be used, and it would also allow for auditing each ERC20 token that's used. This would act as a "defense in depth" mitigation against issues around tokens that allow reentrancy or maliciously designed tokens.

Remediation

Consider only allowing principal from a small whitelist of allowed ERC20 tokens.

6.45 BitcoinCollateralAgentProvider scripts accepts short secrets

Severity Status Remediation Comment
Minor Fix Not Reviewed This is addressed in https://github.com/AtomicLoans/chainabstractionlayer-loans/pull/8, which has not been reviewed by the audit team. (It will be included in an upcoming audit.)

Description

The refundable and seizable collateral scripts in BitcoinCollateralAgentProvider allow secrets to be provided that are less than or equal to 32 bytes, but the Funds contract on the Ethereum side requires that secrets be exactly 32 bytes.

Because these particular secrets are revealed first in the Ethereum smart contract, there's currently no risk of secrets being used that are the wrong length, but it's probably best to validate that secrets are exactly 32 bytes, as is done in the other Bitcoin scripts.

Examples

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L44-L48

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L53-L57

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L70-L74

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L79-L83

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L89-L93

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L146-L150

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L155-L159

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L172-L176

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L181-L185

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

code/bitcoin/packages/bitcoin-collateral-agent-provider/lib/BitcoinCollateralAgentProvider.js:L191-L195

'82', // OP_SIZE
'01', // OP_PUSHDATA(1)
'20', // Hex 32
'a1', // OP_LESSTHANOREQUAL
'69', // OP_VERIFY

Remediation

Instead of OP_LESSTHANOREQUAL OP_VERIFY, use OP_EQUALVERIFY to validate that secrets are exactly 32 bytes in length.

6.46 Loans.sechi reduces clarity and increases gas costs

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/48.

Description

The sechi function here:

code/ethereum/contracts/Loans.sol:L317-L322

function sechi(bytes32 loan, bytes32 usr) private view returns (bytes32 sech) { // Get Secret Hash for Sale Index
	if      (usr == 'A') { sech = sechs[loan].sechAS[sales.next(loan)]; }
	else if (usr == 'B') { sech = sechs[loan].sechBS[sales.next(loan)]; }
	else if (usr == 'C') { sech = sechs[loan].sechCS[sales.next(loan)]; }
	else revert();
}

is used in only one place:

code/ethereum/contracts/Loans.sol:L339

sale = sales.open(loan, loans[loan].bor, loans[loan].lend, loans[loan].agent, sechi(loan, 'A'), sechi(loan, 'B'), sechi(loan, 'C'), tokes[loan], vares[loan]);

There's a needless abstraction here, where hardcoded bytes ('A', 'B', and 'C') are used to communicate which field should be read from a struct. Additionally, the sechi function calls sales.next(loan) repeatedly, when the value shouldn't change.

Remediation

Rewrite without the sechi. Something like the following is clearer and more efficient:

Sechs storage h = sechs[loan];
uint256 i = sales.next(loan);
sale = sales.open(loan, loans[loan].bor, loans[loan].lend, loans[loan].agent,
                  h.sechAS[i], h.sechBS[i], h.sechCS[i], tokes[loan], vares[loan]);

6.47 Redundant struct definitions in Sales

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/37.

Description

Bsig, Lsig, and Asig are identical:

code/ethereum/contracts/Sales.sol:L43-L62

struct Bsig {
	bytes      rsig;  // Borrower Refundable Signature
    bytes      ssig;  // Borrower Seizable Signature
    bytes      rbsig; // Borrower Refundable Back Signature
    bytes      sbsig; // Borrower Seizable Back Signature
}

struct Lsig {
	bytes      rsig;  // Lender Refundable Signature
    bytes      ssig;  // Lender Seizable Signature
    bytes      rbsig; // Lender Refundable Back Signature
    bytes      sbsig; // Lender Seizable Back Signature
}

struct Asig {
	bytes      rsig;  // Agent Refundable Signature
    bytes      ssig;  // Agent Seizable Signature
    bytes      rbsig; // Agent Refundable Back Signature
    bytes      sbsig; // Agent Seizable Back Signature
}

Remediation

Delete all but a single struct (maybe called Sig) and use that everywhere.

6.48 Use specific contract types rather than address

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/44.

Description

Where possible, use properly contract types rather than address. This gives you an increase in type safety and avoids casting.

Examples

code/ethereum/contracts/Loans.sol:L181-L183

constructor (address funds_, address med_) public {
	funds = Funds(funds_);
	med   = Medianizer(med_);

This is better written:

constructor(Funds funds_, Medianizer med_) public {
    funds = funds_;
    med = med_;
}

This code is similar:

code/ethereum/contracts/Loans.sol:L186-L188

function setSales(address sales_) public {
    require(!on);
    sales = Sales(sales_);

Remediation

Use specific constructor types wherever possible.

6.49 Use external instead of public where possible

Severity Status Remediation Comment
Minor Closed This is fixed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/49.

Description

Many of the functions throughout the system marked public could instead be external. Using external has two major advantages:

  1. The compiler can emit less code because there's no need to support two different calling conventions. This reduces code complexity and gas usage.
  2. The code is easier to audit (by professional auditors or end users) because there's no need to worry about internal calls to functions marked external. It reduces the internal surface area, and reducing surface area is a great way to increase security.

Remediation

Use external instead of public everywhere possible.

6.50 Vars functions should be view

Severity Status Remediation Comment
Minor Closed This contract was removed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/33.

Description

The functions in Vars should probably never be state changing, so they should be declared as view:

code/ethereum/contracts/Vars.sol:L4-L9

function APEXT() public returns (uint256);
function ACEXT() public returns (uint256);
function BIEXT() public returns (uint256);
function SALEX() public returns (uint256);
function SETEX() public returns (uint256);
function MINBI() public returns (uint256);

The lack of view on these functions is preventing functions in Loans from being marked as view:

code/ethereum/contracts/Loans.sol:L80-L90

function apex(bytes32 loan)   public returns (uint256) { // Approval Expiration
    return add(loans[loan].born, vares[loan].APEXT());
}

function acex(bytes32 loan)   public returns (uint256) { // Acceptance Expiration
    return add(loans[loan].loex, vares[loan].ACEXT());
}

function biex(bytes32 loan)   public returns (uint256) { // Bidding Expiration
    return add(loans[loan].loex, vares[loan].BIEXT());
}

It would be nice to be able to mark these functions as view too and get some safety from unexpected reentrancy and state changes.

Remediation

Mark these functions' mutability as view.

6.51 Vars should be an interface

Severity Status Remediation Comment
Minor Closed This contract was removed in https://github.com/AtomicLoans/atomicloans-eth-contracts/pull/33.

Description

Vars is a contract, but it doesn't actually contain any implementation. It's probably better declared as an interface.

code/ethereum/contracts/Vars.sol:L3

contract Vars {

Remediation

Use interface Vars { instead.

7 Tool-Based Analysis

Several tools were used to perform automated analysis of the reviewed contracts. These issues were reviewed by the audit team, and relevant issues are listed in the Issue Details section.

7.1 Ethlint

Ethlint is an open source project for linting Solidity code. Only security-related issues were reviewed by the audit team.

The raw output of the Ethlint vulnerability scan can be found here.

7.2 Surya

Surya is an utility tool for smart contract systems. It provides a number of visual outputs and information about structure of smart contracts. It also supports querying the function call graph in multiple ways to aid in the manual inspection and control flow analysis of contracts.

A complete list of functions with their visibility and modifiers can be found here.

Appendix 1 - Disclosure

ConsenSys Diligence (“CD”) typically receives compensation from one or more clients (the “Clients”) for performing the analysis contained in these reports (the “Reports”). The Reports may be distributed through other means, including via ConsenSys publications and other distributions.

The Reports are not an endorsement or indictment of any particular project or team, and the Reports do not guarantee the security of any particular project. This Report does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty. No Report provides any warranty or representation to any Third-Party in any respect, including regarding the bugfree nature of code, the business model or proprietors of any such business model, and the legal compliance of any such business. No third party should rely on the Reports in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset. Specifically, for the avoidance of doubt, this Report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. CD owes no duty to any Third-Party by virtue of publishing these Reports.

PURPOSE OF REPORTS The Reports and the analysis described therein are created solely for Clients and published with their consent. The scope of our review is limited to a review of Solidity code and only the Solidity code we note as being within the scope of our review within this report. The Solidity language itself remains under development and is subject to unknown risks and flaws. The review does not extend to the compiler layer, or any other areas beyond Solidity that could present security risks. Cryptographic tokens are emergent technologies and carry with them high levels of technical risk and uncertainty.

CD makes the Reports available to parties other than the Clients (i.e., “third parties”) -- on its GitHub account (https://github.com/ConsenSys). CD hopes that by making these analyses publicly available, it can help the blockchain ecosystem develop technical best practices in this rapidly evolving area of innovation.

LINKS TO OTHER WEB SITES FROM THIS WEB SITE You may, through hypertext or other computer links, gain access to web sites operated by persons other than ConsenSys and CD. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such web sites' owners. You agree that ConsenSys and CD are not responsible for the content or operation of such Web sites, and that ConsenSys and CD shall have no liability to you or any other person or entity for the use of third party Web sites. Except as described below, a hyperlink from this web Site to another web site does not imply or mean that ConsenSys and CD endorses the content on that Web site or the operator or operations of that site. You are solely responsible for determining the extent to which you may use any content at any other web sites to which you link from the Reports. ConsenSys and CD assumes no responsibility for the use of third party software on the Web Site and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.

TIMELINESS OF CONTENT The content contained in the Reports is current as of the date appearing on the Report and is subject to change without notice. Unless indicated otherwise, by ConsenSys and CD.

You can’t perform that action at this time.