Smart Contract Issues

[1nc0gn170]

Bug Report

Hey Team!

Just had a brief look at swaplace contracts and would like to report a couple of issues!

Issues List:

• Incorrect loop counter implementation using assembly
  In the function Swaplace.acceptSwap(), while iterating over assets, counter is updated incorrectly;

    Asset[] memory assets = swap.asking;

    for (uint256 i = 0; i < assets.length; ) {
     ...
      unchecked {
        assembly {
          i := mload(0x40)
          i := add(i, 1)
          mstore(0x40, i)
        }
      }
    }

    assets = swap.biding;

    for (uint256 i = 0; i < assets.length; ) {
      ...
      unchecked {
        assembly {
          i := mload(0x40)
          i := add(i, 1)
          mstore(0x40, i)
        }
      }
    }

This is actually incorrect, it should be just this

        assembly {
          i := add(i, 1)
        }

Note: Also there is no need to use unchecked for assembly, no validation for overflows happens inside assembly block.

• Using transferFrom instead of safeTransferFrom
  • ERC721
    There are certain smart contracts that do not support ERC721, using transferFrom() may result in the NFT being sent to such contracts and get stuck. Also note that using safeTransferFrom can introduce reentrancy issues use reentrancy guard.
  • ERC20
    As there is no validation for asset addresses, a malicious user might forge a token contract which will not abort the execution on token failure, instead it just fails silently. This will help the attacker gain other persons tokens without even swaping. Use safeTransferFrom also maintain a token whitelist.

For any further help regarding audits please reach out to me here: https://cantina.xyz/u/1nc0gn170
Thanks

[1nc0gn170]

@0xneves @alextnetto Please have a look!

[0xneves]

Hey @1nc0gn170 , thank you so much for your submission!!

As you enlightened us, the mload is wrong - should be mload(i) instead of mload(0x40) but not needed for the current increment since already in memory. This issue you pointed out will need to be considered as well for refactoring the entire test scope as more validation is needed to catch such errors in the future.

The unchecked should be removed from every inline assembly as proposed!

Regarding best practices for token safety, I believe it can be reinforced in the front end instead, especially because safeTransferFrom is not implemented in all types of tokens. For this first version where don't threat every case separately, we will delegate token safety to the user himself. In next versions we indeed will create function such as safeAccetSwap where we call safeTokenTransfer instead.

[blackbeard002]

Hey @0xneves made a PR that closes it #195

[1nc0gn170]

Thanks for the quick response!
Also few minor changes can be enforced!

• Currently an user can create expired swap or swap with 0 assets, by directly passing incorrect values to the function createSwap. Even though the checks were enforced in makeSwap function, user can bypass them by calling createSwap function.
• Avoid initialisation with 0 to save gas.
  for (uint256 i = 0; i < assets.length; ) { => for (uint256 i; i < assets.length; ) {

Thanks!

[alextnetto]

Hey @1nc0gn170, thanks a lot for your efforts in reviewing the code!