twin transfer issue - returnbomb attack

[zajck]

Despite the protective measures currently implemented, the seller can still make redeemVoucher to revert and effectively force the buyer to either cancel the voucher or let it expire (in either case, the buyer loses the cancellation penalty).

There are possible attacks:

• returnbomb attack (this issue)
• malformed return data (twin transfer issue - malformed return data #769)

Returnbomb attack

The twin transfer reverts with insanely long revert data. The return value gets copied into the memory here

boson-protocol-contracts/contracts/protocol/facets/ExchangeHandlerFacet.sol

Line 869 in 9070c04

(success, result) = twin.tokenAddress.call{ gas: gasleft() - reservedGas }(data);

If the result is of the right size it can consume enough gas to make the transaction fail.

Example:

boson-protocol-contracts/contracts/mock/Foreign20.sol

Lines 251 to 263 in a4949ec

	* @title Foreign20 that returns an absurdly long return data
	*
	* @notice Mock ERC-(20) for Unit Testing
	*/
	contract Foreign20ReturnBomb is Foreign20 {
	function transferFrom(address, address, uint256) public virtual override returns (bool) {
	assembly {
	revert(0, 3000000)
	// This is carefully chosen. If it's too low, not enough gas is consumed and contract that call it does not run out of gas.
	// If it's too high, then this contract runs out of gas before the return data is returned.
	}
	}
	}

Recommendation

Use a solution like this https://github.com/nomad-xyz/ExcessivelySafeCall which limits the amount of return data copied to the memory.

[zajck]

Some references:

OZ is not planning to implement this:
OpenZeppelin/openzeppelin-contracts#3544
OpenZeppelin/openzeppelin-contracts#3168

Solidity compiler handles it if the returndata is not used at all (not the case for use).
There is an open issue that addresses the problem.
ethereum/solidity#14467

So in the future, we might be able to handle it without the need for assembly.