Transfer twins should limit the gas it passes to twin contract when making a transfer

[zajck]

transferTwins sends all available gas to twin while doing twin transfers https://github.com/bosonprotocol/boson-protocol-contracts/blob/451dc3d0866ba6694821537803ba32939de34798/contracts/protocol/facets/ExchangeHandlerFacet.sol#L754-L761 https://github.com/bosonprotocol/boson-protocol-contracts/blob/451dc3d0866ba6694821537803ba32939de34798/contracts/protocol/facets/ExchangeHandlerFacet.sol#L771-L779 https://github.com/bosonprotocol/boson-protocol-contracts/blob/451dc3d0866ba6694821537803ba32939de34798/contracts/protocol/facets/ExchangeHandlerFacet.sol#L782-L791

The seller can create a twin that consumes all available gas on transfer. If they make two of them and bundle up them with an offer, buyers can commit to it, but redeemVoucher cannot succeed. The buyer can only cancel the voucher, but then they lose buyerCancelPenalty

The seller can make malicious bundles with new offers or with the existing ones if nobody had committed to them yet.

Recommendation

Limit how much gas can the external contract consume. This will still make twin transfer fail, but redeemVoucher will succeed and the exchange will go into dispute (as implemented in #652 )

Twin region of the protocol was paused to prevent twin creation until the fix is applied.

[imp0wd3r]

Twins are seller controllable. If twins are not trusted, controlling only the gas is not enough. The attacker can keep the balance unchanged when the msg.sender in transferFrom is protocol, so that even if the buyer redeems it, they will not receive twins and will not enter dispute status.

[imp0wd3r]

Also, I submitted several(7) vulnerabilities on immunefi but did not receive any response. Have you received the vulnerability notifications?

[zajck]

@imp0wd3r I can confirm we received your submissions in immunefi and we will respond to you shortly. Thanks for all your submissions and for helping to make the protocol better and more secure.

And to respond to your comment about checking the balance. The buyer can always manually raise a dispute if the number of twins they receive does not match the expectation.

True, we could check the balance before and after the twin transfer, but the only reliable case, where we could conclude that the twin transfer was unsuccessful is if the twin is ERC20 and the balance after the transfer is 0.

In another case, where the twin is ERC20 and the difference between the balance before and after does not match the expected twin amount, that does not necessarily mean the transfer is invalid - the twin token might take a fee and both buyer and seller are ok with it. We don't just want to raise a dispute if that happens.

Furthermore, if we checked the balance of ERC20, it would make also sense to do something similar for ERC721 and ERC1155 twins. But for those two, the recipient could be a smart contract which would immediately forward the received token to another address. So protocol can not reliably compare the ERC721 and ERC1155 balances or check the token ownership.

Currently, the dispute will be raised automatically only if the call to twin reverts or if ERC20 returns false. In any other cases, the buyer will have to raise a dispute manually.

[imp0wd3r]

@zajck Thank you for your explanation. It was my mistake to think that if the transfer is successful, it cannot be disputed. Therefore, I can ignore a vulnerability that I submitted on Immunefi and will indicate so in its comments.

[imp0wd3r]

Also, may I ask if this vulnerability was discovered on Immuefi or found by your own inspection? I am curious.

[mischat]

@imp0wd3r, thanks for disclosing all of the bugs you submitted on immunefi, we have been working through them so that we can get back to you. Assuming that it is you on immunefi creating the bug reports. I think it is unfair to say that we haven't responded to you at all, as we have on some of the issues. Thanks for the bug hunting, and you will hear back from us!

[mischat]

Re: where this bug was found ...

The core protocol devs, our dapp building team, are constantly finding things, this in conjunction with period audits, and our bug bounty program, and the diamond pattern will allow us to build a solid and useful protocol :)

[imp0wd3r]

@mischat Thank you for your reply. I think my expression was incorrect. It's that I haven't received a response within this week, and mainly because I'm anxious to know the result. Sorry.

[mischat]

Yes, you did submit a few things, and we had a few other submissions too, you will hear from us shortly. thanks

[imp0wd3r]

@mischat @zajck Hi, I submitted two more vulnerabilities on Immunefi over a week ago. Have you processed them yet?

[imp0wd3r]

@mischat @zajck Hi, I submitted two more vulnerabilities on Immunefi over a week ago. Have you processed them yet?

There is still one unanswered report, it has been two weeks already. Can you confirm if you have received the vulnerability? The subject of the vulnerability starts with (Resubmit).

[mischat]

@imp0wd3r we have been looking at the stuff coming in to immunefi, thank you. We will respond in due course on that platform. Like I said before, we are a small team, we are building up to a new release / audit of the protocol which starts next week, and we do like to properly process and read all reports that are submitted to us via immunefi.

thank you for your patience.

[imp0wd3r]

@imp0wd3r we have been looking at the stuff coming in to immunefi, thank you. We will respond in due course on that platform. Like I said before, we are a small team, we are building up to a new release / audit of the protocol which starts next week, and we do like to properly process and read all reports that are submitted to us via immunefi.

thank you for your patience.

Hi everyone, I may have discovered a critical vulnerability. I have submitted it on immunefi. Please take a look as soon as possible if you have the time, as it could have serious consequences if it is indeed a vulnerability.