Loading...
Home / Research / databroker-global-audit /
  Share   | 399

Databroker.global Audit

October 17, 2021    |    By Jonathan Becker

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.

Disclaimer

As of October 17, 2021, this smart contract has been audited to the best of my abilities keeping security patterns & best practices in mind. The findings of this audit are not guaranteed to be exhaustive, and there may still be issues within the contract itself. I take no responsibility for Databroker's contract security, and only act as a third-party auditor.

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.

0x01. Severity Level Reference

CRITICAL

Issues marked with a critical severity tag must be fixed as soon as possible. These issues may break the contract altogether if not resolved.

HIGH

Issues 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.

MEDIUM

Issues 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.

LOW

Issues 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.

0x02. Security Issues

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

    CRITICAL

    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.

    _swapTokens(
      sellerAmountInDTX,
      sellerAmountOutMin,
      DTXToUSDTPath,
      _payoutWalletAddress,
      block.timestamp + _uinswapDeadline
    );
    
    require(
      _dtxToken.transfer(_dtxStakingAddress, stakingCommission),
      "DTX transfer failed for _dtxStakingAddress"
    );
    
    require(
      _dtxToken.transfer(deal.platformAddress, databrokerCommission),
      "DTX transfer failed for platformAddress"
    );
    
    _pendingDeals.remove(dealIndex);
    deal.payoutCompleted = true;

    Becomes:

    _pendingDeals.remove(dealIndex);
    deal.payoutCompleted = true;
    
    _swapTokens(
      sellerAmountInDTX,
      sellerAmountOutMin,
      DTXToUSDTPath,
      _payoutWalletAddress,
      block.timestamp + _uinswapDeadline
    );
    
    require(
      _dtxToken.transfer(_dtxStakingAddress, stakingCommission),
      "DTX transfer failed for _dtxStakingAddress"
    );
    
    require(
      _dtxToken.transfer(deal.platformAddress, databrokerCommission),
      "DTX transfer failed for platformAddress"
    );
  • settleDeclinedDeal is Vulnerable To Reentrancy [ SWC-107 ] [ Code Reference ]

    CRITICAL

    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.

    _swapTokens(
      amountsIn[0],
      buyerAmountOutMin,
      DTXToUSDTPath,
      _payoutWalletAddress,
      block.timestamp + _uinswapDeadline
    );
    
    deal.payoutCompleted = true;
    _pendingDeals.remove(dealIndex);

    Becomes:

    deal.payoutCompleted = true;
    _pendingDeals.remove(dealIndex);
    
    _swapTokens(
      amountsIn[0],
      buyerAmountOutMin,
      DTXToUSDTPath,
      _payoutWalletAddress,
      block.timestamp + _uinswapDeadline
    );
  • Deals Auto-flagged As Accepted [ Code Reference ]

    MEDIUM

    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 ]

    LOW

    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 ]

    LOW

    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 ]

    LOW

    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).

    modifier validDestination( address platformAddress ) {
        require(platformAddress != address(0x0));
        require(platformAddress != address(this) );
        _;
    }

0x03. 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.

    bytes32 private constant OWNER_ROLE = keccak256("OWNER_ROLE");
    bytes32 private constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
    bytes32 private constant OWNER_ROLE = "b19546dff01e856fb3f010c267a7b1c60363cf8a4664e21cc89c26224620214e";
    bytes32 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

    function payout(uint256 dealIndex) public whenNotPaused hasAdminRole {
      ...
    }

    while others may be

    function calculateTransferAmount(
      uint256 dealIndex,
      address[] memory DTXToUSDTPath
    )
      public
      view
      isDealIndexValid(dealIndex)
      returns (
        uint256,
        uint256,
        uint256
      )
    {
      ...
    }

    or

    function createDeal(
      string memory did,
      address buyerId,
      address sellerId,
      bytes32 dataUrl,
      uint256 amountInUSDT,
      uint256 amountOutMin,
      uint256 platformPercentage,
      uint256 stakingPercentage,
      uint256 lockPeriod,
      address platformAddress
    ) public whenNotPaused hasAdminRole {
      ...
    }

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

0x04. 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.

As of October 17, 2021, the above audit reflects my current understanding of solidity best practices and security to the best of my knowledge.


0x05. Resources & Citations