search

bosonprotocol / boson-protocol-contracts

Boson Protocol V2 (latest)
https://bosonprotocol.io/
GNU General Public License v3.0
32 stars 8 forks source link

twin transfer issue - returnbomb attack #768

Closed zajck closed 1 year ago

zajck commented 1 year ago

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

The twin transfer reverts with insanely long revert data. The return value gets copied into the memory here https://github.com/bosonprotocol/boson-protocol-contracts/blob/9070c0484634cec7bec793808a43ca20616d889a/contracts/protocol/facets/ExchangeHandlerFacet.sol#L869

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

Example: https://github.com/bosonprotocol/boson-protocol-contracts/blob/a4949ec7969df0d3779136bd9eaa7ddfef8f116b/contracts/mock/Foreign20.sol#L251-L263

Recommendation

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

zajck commented 1 year ago

Some references:

OZ is not planning to implement this: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3544 https://github.com/OpenZeppelin/openzeppelin-contracts/pull/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. https://github.com/ethereum/solidity/issues/14467

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