ExchangeHandlerFacet Manual Review Findings

EHF-01M: Improper Invocation of EIP-20 transfer

Type	Severity	Location
Standard Conformity		ExchangeHandlerFacet.sol:L649-L656

Description:

The linked statement does not properly validate the returned bool of the EIP-20 standard transfer function. As the standard dictates, callers must not assume that false is never returned.

Impact:

If the code mandates that the returned bool is true, this will cause incompatibility with tokens such as USDT / Tether as no such bool is returned to be evaluated causing the check to fail at all times. On the other hand, if the token utilized can return a false value under certain conditions but the code does not validate it, the contract itself can be compromised as having received / sent funds that it never did.

Example:

Copy
649(success, result) = twin.tokenAddress.call(
650    abi.encodeWithSignature(
651        "transferFrom(address,address,uint256)",
652        seller.operator,
653        sender,
654        twin.amount
655    )
656);

Recommendation:

Since not all standardized tokens are EIP-20 compliant (such as Tether / USDT), we advise a safe wrapper library to be utilized instead such as SafeERC20 by OpenZeppelin to opportunistically validate the returned bool only if it exists.

Alleviation (44009967e4):

The transferFrom function invoked in the referenced statement is still performed using its unsafe counterpart whilst a bool may never be yielded by the specified tokenAddress. We advise the Boson Protocol team to revisit this exhibit.

Alleviation (6dae5d2602):

After re-visiting this exhibit, the Boson Protocol team highlighted that the success and result variable are both utilized in the code similarly to how the safeTransferFrom function of OpenZeppelin behaves and as such the code works as intended. In light of this, we consider this exhibit nullified as it never presented an issue.