/Paydece-Bug-Bounty

Primary LanguageSolidity

Contract

Escrow contract addresses

  • (OLD) BSC: 0xaa15a71515116ae395784e100f7ccc2385680e8d
  • Polygon Mainnet: 0xde8248e12ca4fdff2c638b0c604e1ea6943cc9bd
  • RSK Mainnet: 0xDe8248E12Ca4FDff2c638b0c604E1eA6943Cc9Bd
  • Ethereum Mainnet: 0xde8248e12ca4fdff2c638b0c604e1ea6943cc9bd

Optimizations

  • A lot (most) of the dependencies can be removed. This will reduce deployment costs substancially.
  • Most getters should be removed. They increase contract complexity and bytecode size. this will reduce deployment costs and contract complexity
  • getFeesAvailable() is useless and should be removed. feesAvailable is already public.
  • getAllowance() is useless and should be removed. This balance should be queried directly from the ERC20 contract.
  • There is no need to delete stale orders from on-chain memory. It serves no practical purpose and increases gas cost.

Bugs

  • LEVEL 1 = NOT SEVERE. Should not be ignored, but can be.
  • LEVEL 2 = SEVERE. Exploit that can lead to lose of funds. Must be fixed before deployment.
  • LEVEL 3 = FATAL. Contract execution will fail. Must be fixed before deployment.

1 (LEVEL 1)

createEscrow call is open to anyone, not just contract owner. That means anyone can make an escrow on behalf of anyone.

elchango.eth says this is intended functionality. In that case the functions comment is misleading. https://discord.com/channels/952926531846541332/992764249493483650/1001261262400925787

2 (LEVEL 1)

createEscrow doesn't check that _seller != address(0) and _buyer != address(0)

3 (LEVEL 1)

Inside releaseEscrow the require message says "USDT has not been deposited" which means the escrow is now only useful for USDT token

4 (LEVEL 2)

refundBuyer function is prone to re-entrance attacks. This is irrelavent (as of right now) given the current escrow contract only works with a preset ERC20 contract. If the token contract could be set by the user then it could be set to a malicious contract that performs the rentrence attack and drains all buyer funds.

example:

attacker calls escrow.refundBuyer(0) which calls maliciousERC20.safeTransfer. the safeTransfer calls escrow.refundBuyer(1). This will make the escrow call maliciousERC20.safeTransfer before updating its internal balance. This loops endlessly until the escrow contract is completely drained.

4 (LEVEL 2)

cancelEscrow function is prone to re-entrance attacks. Same exact way as refundBuyer function.

5 (LEVEL 1)

onlyBuyer and onlySeller modifiers do not check that msg.sender != address(0). remember if escrows[_orderId] is undefined then escrows[_orderId].buyer will be address(0) which is a potential bug.

6 (LEVEL 2)

All ERC20 token transfers are assumed to be successful. This is not always the case and checks should be implemented accordingly to avoid indefinetly locking funds in the escrow contract.

7 (LEVEL 3)

releaseEscrow reverts with arithmetic overflow/underflow. The equations need to be completely re-worked.

8 (LEVEL 2)

createEscrow transactions can be front run to ceate fradulant transactions. All funds are at risk.

Final Remarks

  • It is highly recommend that orderId be counter managed solely by the escrow contract. releaseEscrow should increment it (orderId++). Allowing it to be set by the caller is a massive attack surface.

  • values like 100000000000000000000 should be declared as constants.