Using transferFrom instead of safeTransferFrom

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L344

Vulnerability details

Impact

Using transfer and transferFrom instead of their safe alternatives may result in transactions fail silently.

Proof of Concept

Using token transferFrom functions instead of safeTransferFrom (https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L344)
which is highly discouraged and can cause NFTs to be stuck in the case of the transaction not reverting on failed transfers, also, in this case, because the option tokens are burnt in the process (https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L334), calling the function a second time would not mitigate the issue, since the option tokens would be gone. There’s also precedents of this vulnerability as seen here code-423n4/2022-01-trader-joe-findings#12

Tools Used

Manual code review

Recommended Mitigation Steps

We suggest you to check all of your contracts and fix this issue by implementing safeTransfer and safeTransferFrom instead of transfer and transferFrom where applicable.

[outdoteth]

use safeTransferFrom to prevent stuck NFTs: #38