The Arwen team asked us to review their CWC Ethereum Contracts. We looked at the code and now publish our results.
The audited commit is 89c1b32f343131ef686f1fdbc6e90e000ddc0505 and the files included in scope were Escrow.sol, EscrowFactory.sol, EscrowLibrary.sol, Migrations.sol, and TestToken.sol.
We commend the Arwen team for the well written code and the organization of the project. The contracts solve a complex problem with a relatively simple implementation.
Here are our assessment and recommendations, in order of importance.
Update: Our team also reviewed the the changes made to the EscrowFactory to use create2. The last commit reviewed was d3d46ac and no additional issues were found. Arwen has asked us to note that while the audit covered the entirety of the smart contract including ERC20 compatibility code, the full Arwen software stack does not support ERC20s or ERC777 at this time. At this time, Arwen is only adding support for ETH. Arwen will of course address and mitigate all issues found.
Issues
Critical
[C01] Escrow can be DoSed and funds stolen when escrowing ERC777 tokens
In the ERC20Escrow contract, the send function makes an ERC20 transfer to the specified address. Some ERC20 tokens, such as Augur’s REP, also follow the ERC777 standard which will optionally call hooks in the sending and receiving contracts. In this case, an attacker may specify their escrowerReserve address to be an ERC777TokensRecipient contract that is set to revert all attempts to send it ERC777 tokens. This will effectively freeze the escrow contract ensuring neither party can withdraw. After the escrowTimelock has expired, the attacker can set the contract at the escrowerReserve address to receive funds once again and call the forceRefund function to reclaim all of the funds. Consider switching to pull payments for ERC20 transfers or ensure that ERC777 tokens are not used with the escrow contract.
Update: Fixed in 466af85 and 34d161d
High
[H01] Attacker can create unintended EscrowFunded events
In the EscrowLibrary contract, the checkFunded function checks if an escrow is funded and emits its balance in an event if it is. Because there are no protections on the function, it can be called by any address to trigger an EscrowFunded event. If off-chain systems are using this event to trigger any critical actions, a maliciously triggered EscrowFunded event may lead to a loss of funds. Consider using msg.sender instead of passing in the escrowAddress parameter to ensure EscrowFunded events can only be triggered by the escrows themselves.
Update: Fixed in f693df6. The Arwen team pointed out that multiple EscrowFunded events could still be created by triggering the Escrow contract’s payable function multiple times. The team will ensure that multiple EscrowFunded events are handled gracefully off-chain.
Medium
[M01] Silent send failures
send is used here and here to distribute funds. Consider adding logic to show an explicit failure upon send, otherwise these sends will fail silently.
The simplest solution may be to log the return bool from the send call in an event. A more involved solution may be to allow for an alternative way to distribute funds. If one (or both) of the `send` functions from the second set of sends were to fail, the funds would be stuck in the contract with no way for the owners to get out. Consider adding a method for the rightful owners to pull their funds out.
An increasingly common case where these send functions may fail is with the use of contract-based accounts. Since send only has a 2300 gas stipend, there is a chance that a contract-based account recipient with a heavy fallback function causes the send to fail. Additionally, the introduction of EIP-1884 in the upcoming Istanbul Hard Fork changes opcode costs to potentially make these contract-based account fallback functions more expensive.
Update: Fixed in 7153a2d and 466af85
[M02] Return value of transfer is not checked
In the Erc20Escrow contract’s send function, an ERC20 transfer is made but the return value is never checked. Some ERC20 implementations will return `false` instead of reverting when the `transfer` fails. Consider checking the return value to ensure transfers do not fail silently. Alternatively, use OpenZeppelin’s SafeERC20 wrapper to add compatibility with some non-standard ERC20 tokens.
Update: Fixed in 7153a2d and 466af85
Low
[L01] Use SafeMath
There are a number of arithmetic operations throughout the code. Consider using OpenZeppelin’s SafeMath in order to guarantee no overflows or underflows during these operations.
Update: Fixed in cb07d4f
Notes
- Consider updating to a newer version of Solidity throughout the project. There have been bug fixes in more recent versions of the language, making these versions preferable over older ones.
One issue of critical severity and one issue of high severity were found. Some changes were proposed to follow best practices and reduce the potential attack surface.
