Databroker.global Smart Contract Audit

databroker

This audit was performed on the DatabrokerGlobal/Polygon-migration GitHub repository. The repository version used for this audit was commit e3cfa08c0298ba96e7d15c08f50b1f2982cfa0e9.

The goal of this audit is to review Databroker's DatabrokerDeals.sol smart contract and find and demonstrate potential security issues within the solidity contract. This report will also focus on current Solidity & security best practices.

Documentation & Whitepaper

The contents of this audit are based on the whitepaper provided by Databroker.global within their GitHub. In order to provide a quality report of the security and efficiency of their smart contract, I have studied their whitepaper extensively to get a feel for how their system is supposed to function.

Severity Level Reference

Severity Level	Description
	Findings marked with a critical severity tag must be fixed as soon as possible. These issues may break the contract altogether if not resolved.
	Findings marked with a high severity tag should be fixed as soon as possible, because it is likely they will cause problems in production if left unresolved.
	Findings marked with a medium severity tag should be fixed soon, but it is not extremely urgent. These issues have the potential to cause problems in production.
	Findings marked with a low severity tag can remain unfixed. These are unlikely to cause any problems in production, but resolving them could improve contract efficiency.

Security Issues

Payout is Vulnerable To Reentrancy [ SWC-107 ] [ Code Reference ]

Although payout(...) requires the role ADMIN_ROLE in order to be called, it is vulnerable to reentrancy when called because a state change is made after external calls are made. This allows this payout(...) function to be called multiple times before the deal is marked as completed.

In order to properly fix this, the state change should be moved above the external calls. Should the require statements fail after these state changes, they will be reverted.

Fixed in 827b1d5a87d311197ea872da50f4230ed1811f17.

snippet.sol
1_swapTokens( 2 sellerAmountInDTX, 3 sellerAmountOutMin, 4 DTXToUSDTPath, 5 _payoutWalletAddress, 6 block.timestamp + _uinswapDeadline 7); 8 9require( 10 _dtxToken.transfer(_dtxStakingAddress, stakingCommission), 11 "DTX transfer failed for _dtxStakingAddress" 12); 13 14require( 15 _dtxToken.transfer(deal.platformAddress, databrokerCommission), 16 "DTX transfer failed for platformAddress" 17); 18 19_pendingDeals.remove(dealIndex); 20deal.payoutCompleted = true;

Becomes:

snippet.sol
1_pendingDeals.remove(dealIndex); 2deal.payoutCompleted = true; 3 4_swapTokens( 5 sellerAmountInDTX, 6 sellerAmountOutMin, 7 DTXToUSDTPath, 8 _payoutWalletAddress, 9 block.timestamp + _uinswapDeadline 10); 11 12require( 13 _dtxToken.transfer(_dtxStakingAddress, stakingCommission), 14 "DTX transfer failed for _dtxStakingAddress" 15); 16 17require( 18 _dtxToken.transfer(deal.platformAddress, databrokerCommission), 19 "DTX transfer failed for platformAddress" 20);
settleDeclinedDeal is Vulnerable To Reentrancy [ SWC-107 ] [ Code Reference ]

Although settleDeclinedDeal(...) requires the role ADMIN_ROLE, it is vulnerable to reentrancy when called because a state change is made after external calls are made. This allows this settleDeclinedDeal(...) function to be called multiple times before the deal is marked as completed.

In order to properly fix this, the state change should be moved above the external calls. Should the require statements fail after these state changes, they will be reverted.

Fixed in 827b1d5a87d311197ea872da50f4230ed1811f17.

snippet.sol
1_swapTokens( 2 amountsIn[0], 3 buyerAmountOutMin, 4 DTXToUSDTPath, 5 _payoutWalletAddress, 6 block.timestamp + _uinswapDeadline 7); 8 9deal.payoutCompleted = true; 10_pendingDeals.remove(dealIndex);

Becomes:

snippet.sol
1deal.payoutCompleted = true; 2_pendingDeals.remove(dealIndex); 3 4_swapTokens( 5 amountsIn[0], 6 buyerAmountOutMin, 7 DTXToUSDTPath, 8 _payoutWalletAddress, 9 block.timestamp + _uinswapDeadline 10);
Deals Auto-flagged As Accepted [ Code Reference ]

When calling createDeal(...), a new Deal object is created with the accepted bool set to true. This should automatically be set to false, since the buyer has not accepted the deal yet. Although there is a timelock implemented, it is bad practice to mark this deal as accepted before the buyer has called acceptDeal(). If the timelock were to expire before the buyer had the opportunity to declineDeal(), the seller would be able to request a payout without the proper flow being handled.
Timestamp Dependence [ SWC-116 ] [ Code Reference ]

Timestamps can be manipulated by the miner. It is generally safe to use block.timestamp, since Geth and Parity reject timestamps that are more than 15 seconds in the future. Since Databroker uses a 30 day timelock, this shouldn't be a problem, but should be kept in mind.
Floating Pragma [ SWC-103 ] [ Code Reference ]

Contracts should be deployed using the exact compiler version that they have been tested the most with in order to prevent being deployed with a compiler version that may have undiscovered bugs or vulnerabilities. This is best practice when deploying contracts.
Check Deal platformAddress [ Code Reference ]

When calling createDeal it is best practice to ensure that platformAddress is not 0x0 or address.this(). Adding a modifier that requires these two conditions would prevent token loss.

Partially addressed in 827b1d5a87d311197ea872da50f4230ed1811f17. Still does not account for address(this).

snippet.sol
1modifier validDestination( address platformAddress ) { 2 require(platformAddress != address(0x0)); 3 require(platformAddress != address(this) ); 4 _; 5}

Performance & Best Practice Issues

Contract Size [ EIP-170 ]

The raw contract code exceeds 24576 bytes, and may not deploy on mainnet correctly if not optimized. It may need to be optimized in order to deploy it.
Computed Constant [ Code Reference ]

It is typically good practice to write the value of predefined constants if you know them before compiling. Using functions such as keccak256() when you can compute the hash beforehand is excessive and will waste gas.

snippet.sol
1bytes32 private constant OWNER_ROLE = keccak256("OWNER_ROLE"); 2bytes32 private constant ADMIN_ROLE = keccak256("ADMIN_ROLE");

snippet.sol
1bytes32 private constant OWNER_ROLE = "b19546dff01e856fb3f010c267a7b1c60363cf8a4664e21cc89c26224620214e"; 2bytes32 private constant ADMIN_ROLE = "a49807205ce4d355092ef5a8a18f56e8913cf4a201fbe287825b095693c21775";
Seller Can Also Be The Buyer [ Code Reference ]

Assuming you do not want users to be able to create deals with themselves, there should be a require statement after line 142 that requires buyerId and sellerId to be different from each other. This shouldn't cause problems with the contract itself, but would reduce gas consumption.
Documentation Issues

Proper NatSpec documentation missing from all functions within DatabrokerDeals.sol. Consider adding additional documentation, including inline documentation for ease of readability for consumers.
Variable Issues

The variable _uinswapDeadline is misspelled and should be corrected to _uniswapDeadline in order to maintain professionalism and keep the contract readable and coherent for consumers.``

Fixed in 827b1d5a87d311197ea872da50f4230ed1811f17.
Contract Readability & Consistency

This contract uses many different coding styles that should be made uniform for ease of readabity for the consumer. For example, this contract has some functions structured as

snippet.sol
1function payout(uint256 dealIndex) public whenNotPaused hasAdminRole { 2 ... 3}

while others may be

snippet.sol
1function calculateTransferAmount( 2 uint256 dealIndex, 3 address[] memory DTXToUSDTPath 4) 5 public 6 view 7 isDealIndexValid(dealIndex) 8 returns ( 9 uint256, 10 uint256, 11 uint256 12 ) 13{ 14 ... 15}

or

snippet.sol
1function createDeal( 2 string memory did, 3 address buyerId, 4 address sellerId, 5 bytes32 dataUrl, 6 uint256 amountInUSDT, 7 uint256 amountOutMin, 8 uint256 platformPercentage, 9 uint256 stakingPercentage, 10 uint256 lockPeriod, 11 address platformAddress 12) public whenNotPaused hasAdminRole { 13 ... 14}

These coding styles must be made uniform in order to allow for users to read and interpret your code with ease.

Conclusion

A total of 12 issues & recommendations were found within DatabrokerDeals.sol, two of which were of critical severity and one of which was of medium severity. The remaining nine issues were either of low severity or were recommendations in order to adhere to solidity best practice.

Note that as of the date of publishing, the contents of this document reflect my current understanding of known security patterns regarding smart contract security. The findings of this audit should not be considered to be exhaustive, and there may still be issues within the contract itself. This audit is not a guarantee of security. I take no responsibility for future contract security, and only act as a third-party auditor.