Crytic.io security analysis of the HQ20 contracts

[montyly]

Hi,

I am a security researcher from Trail of Bits. I ran crytic.io on your codebase, and some of the findings might be of interest to you.

Crytic is a GitHub application that provides continuous assurance for smart contracts. It runs a set of bugs and optimizations detectors on every commit and allows evaluating custom properties.

Issue 1 : Lack of return value check can lead to unexpected results

Description

Several calls do not check the return value. The lack of return value check is error-prone and might lead to unexpected results.

For example, invest does not check the return value of transferFrom:

contracts/contracts/issuance/Issuance.sol

Lines 69 to 78 in a0dcfe2

	function invest(uint256 _amount) external virtual {
	require(
	currentState == "OPEN",
	"Not open for investments."
	);
	require(
	_amount.mod(issuePrice) == 0,
	"Fractional investments not allowed."
	);
	IERC20(currencyToken).transferFrom(msg.sender, address(this), _amount);

As a result, if Invest were to be used with a currencyToken that returns false and does not revert in case of insufficient balance (which is the case for many ERC20 tokens), anyone could invest for free.

The complete list of missing calls is:

ERC20MultiDividendable.releaseDividends(uint256,address) (drafts/token/ERC20MultiDividendable.sol#39-48) ignores return value by IERC20(dividendToken).transferFrom(msg.sender,address(this),amount) (drafts/token/ERC20MultiDividendable.sol#43)
ERC20MultiDividendable.claimDividends(address,address) (drafts/token/ERC20MultiDividendable.sol#56-70) ignores return value by IERC20(dividendToken).transfer(account,owing) (drafts/token/ERC20MultiDividendable.sol#68)
Issuance.invest(uint256) (issuance/Issuance.sol#69-85) ignores return value by IERC20(currencyToken).transferFrom(msg.sender,address(this),_amount) (issuance/Issuance.sol#78)
Issuance.claim() (issuance/Issuance.sol#87-105) ignores return value by _issuanceToken.mint(msg.sender,amount.divd(issuePrice,_issuanceToken.decimals())) (issuance/Issuance.sol#101-104)
Issuance.cancelInvestment() (issuance/Issuance.sol#110-123) ignores return value by IERC20(currencyToken).transfer(msg.sender,amount) (issuance/Issuance.sol#121)
Issuance.withdraw(address) (issuance/Issuance.sol#153-161) ignores return value by IERC20(currencyToken).transfer(_wallet,amountRaised) (issuance/Issuance.sol#160)
UniswapExchange.initializeExchange(uint256) (exchange/UniswapExchange.sol#82-98) ignores return value by token.transferFrom(msg.sender,address(this),_tokenAmount) (exchange/UniswapExchange.sol#97)
UniswapExchange.investLiquidity(uint256) (exchange/UniswapExchange.sol#302-333) ignores return value by token.transferFrom(msg.sender,address(this),tokensRequired) (exchange/UniswapExchange.sol#331)
UniswapExchange.divestLiquidity(uint256,uint256,uint256) (exchange/UniswapExchange.sol#339-371) ignores return value by token.transfer(msg.sender,tokensDivested) (exchange/UniswapExchange.sol#368)
UniswapExchange.ethToToken(address,address,uint256,uint256) (exchange/UniswapExchange.sol#387-410) ignores return value by token.transfer(recipient,tokensOut) (exchange/UniswapExchange.sol#408)
UniswapExchange.tokenToEth(address,address,uint256,uint256) (exchange/UniswapExchange.sol#412-436) ignores return value by token.transferFrom(buyer,address(this),tokensIn) (exchange/UniswapExchange.sol#433)
UniswapExchange.tokenToTokenOut(address,address,address,uint256,uint256) (exchange/UniswapExchange.sol#438-470) ignores return value by token.transferFrom(buyer,address(this),tokensIn) (exchange/UniswapExchange.sol#468)
UniswapExchange.tokenToTokenOut(address,address,address,uint256,uint256) (exchange/UniswapExchange.sol#438-470) ignores return value by exchange.tokenToTokenIn.value(ethOut)(recipient,minTokensOut) (exchange/UniswapExchange.sol#469)
Democratic.propose(bytes) (voting/Democratic.sol#46-57) ignores return value by proposals.add(address(voting)) (voting/Democratic.sol#55)
Democratic.onlyProposal() (voting/Democratic.sol#29-33) ignores return value by proposals.remove(msg.sender) (voting/Democratic.sol#32)
ERC721._mint(address,uint256) (@openzeppelin/contracts/token/ERC721/ERC721.sol#397-408) ignores return value by _holderTokens[to].add(tokenId) (@openzeppelin/contracts/token/ERC721/ERC721.sol#403)
ERC721._mint(address,uint256) (@openzeppelin/contracts/token/ERC721/ERC721.sol#397-408) ignores return value by _tokenOwners.set(tokenId,to) (@openzeppelin/contracts/token/ERC721/ERC721.sol#405)
ERC721._burn(uint256) (@openzeppelin/contracts/token/ERC721/ERC721.sol#415-433) ignores return value by _holderTokens[owner].remove(tokenId) (@openzeppelin/contracts/token/ERC721/ERC721.sol#428)
ERC721._burn(uint256) (@openzeppelin/contracts/token/ERC721/ERC721.sol#415-433) ignores return value by _tokenOwners.remove(tokenId) (@openzeppelin/contracts/token/ERC721/ERC721.sol#430)
ERC721._transfer(address,address,uint256) (@openzeppelin/contracts/token/ERC721/ERC721.sol#442-457) ignores return value by _holderTokens[from].remove(tokenId) (@openzeppelin/contracts/token/ERC721/ERC721.sol#451)
ERC721._transfer(address,address,uint256) (@openzeppelin/contracts/token/ERC721/ERC721.sol#442-457) ignores return value by _holderTokens[to].add(tokenId) (@openzeppelin/contracts/token/ERC721/ERC721.sol#452)
ERC721._transfer(address,address,uint256) (@openzeppelin/contracts/token/ERC721/ERC721.sol#442-457) ignores return value by _tokenOwners.set(tokenId,to) (@openzeppelin/contracts/token/ERC721/ERC721.sol#454)
OneTokenOneVote.vote(uint256) (voting/OneTokenOneVote.sol#74-78) ignores return value by votingToken.transferFrom(msg.sender,address(this),_votes) (voting/OneTokenOneVote.sol#75)
OneTokenOneVote.cancel() (voting/OneTokenOneVote.sol#81-86) ignores return value by votingToken.transfer(msg.sender,count) (voting/OneTokenOneVote.sol#84)
EnergyMarket.produce(uint256) (examples/energy/EnergyMarket.sol#78-86) ignores return value by this.transfer(msg.sender,getProductionPrice(_time)) (examples/energy/EnergyMarket.sol#80-83)
EnergyMarket.consume(uint256) (examples/energy/EnergyMarket.sol#93-102) ignores return value by this.transferFrom(msg.sender,address(this),getConsumptionPrice(_time)) (examples/energy/EnergyMarket.sol#95-99)
OneManOneVote.vote() (voting/OneManOneVote.sol#73-76) ignores return value by votes.add(msg.sender) (voting/OneManOneVote.sol#74)
OneManOneVote.cancel() (voting/OneManOneVote.sol#79-82) ignores return value by votes.remove(msg.sender) (voting/OneManOneVote.sol#80)
Classifieds.executeTrade(uint256) (classifieds/Classifieds.sol#82-92) ignores return value by currencyToken.transferFrom(msg.sender,trade.poster,trade.price) (classifieds/Classifieds.sol#88)
DAO.investVenture(address,uint256) (examples/dao/DAO.sol#68-75) ignores return value by ventures.add(venture) (examples/dao/DAO.sol#72)
DAO.cancelVenture(address) (examples/dao/DAO.sol#91-96) ignores return value by ventures.remove(venture) (examples/dao/DAO.sol#95)
IssuanceEth.claim() (issuance/IssuanceEth.sol#59-77) ignores return value by _issuanceToken.mint(msg.sender,amount.divd(issuePrice,_issuanceToken.decimals())) (issuance/IssuanceEth.sol#73-76)

Exploit Scenario

Invest is used with BAT. BAT returns false and does not revert in case of failure. As a result, Eve is able to invest without sending tokens

Recommendations

Check all the return values

Crytic Report

Issue 2 : Reentrancies might lead to theft of tokens

Description

Classifieds calls transferFrom on external tokens without following the check-effects-interaction pattern. This leads to potential reentrancies that can be exploited by an attacker if the destination token has a callback mechanism (for example: an ERC777 contracts).

This is similar to the recent uniswap and lendf.me hacks

The reentracies are in:

Reentrancy in Classifieds.cancelTrade(uint256) (classifieds/Classifieds.sol#98-111):
	External calls:
	- itemToken.transferFrom(address(this),trade.poster,trade.item) (classifieds/Classifieds.sol#108)
	State variables written after the call(s):
	- trades[_trade].status = Cancelled (classifieds/Classifieds.sol#109)
Reentrancy in Classifieds.executeTrade(uint256) (classifieds/Classifieds.sol#82-92):
	External calls:
	- currencyToken.transferFrom(msg.sender,trade.poster,trade.price) (classifieds/Classifieds.sol#88)
	- itemToken.transferFrom(address(this),msg.sender,trade.item) (classifieds/Classifieds.sol#89)
	State variables written after the call(s):
	- trades[_trade].status = Executed (classifieds/Classifieds.sol#90)

Exploit Scenario

Classifieds is used with an ERC777 contract. Eve exploits the reentrancy in cancelTrade to drain the contract’s funds.

Recommendations

Follow the check-after-effect pattern

Crytic Report

I would also recommend to subscribe to Crytic, it is currently free for three months.

Additionally, if you are looking for a security audit, we would be happy to help you secure your code.

Let me know if you have any questions.