Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Use of transferFrom() instead of safeTransferFrom() #14

Closed
code423n4 opened this issue May 10, 2022 · 1 comment
Closed

Use of transferFrom() instead of safeTransferFrom() #14

code423n4 opened this issue May 10, 2022 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L199

Vulnerability details

Impact

In Cally.sol the createVault() function calls transferFrom() on a ERC721 token. This does not ensure that the ERC721 token is not sent to an address that is not able to properly support it which could result in the loss of the token.

Proof of Concept

https://github.com/code-423n4/2022-05-cally/blob/main/contracts/src/Cally.sol#L199.

Tools Used

Manual code review

Recommended Mitigation Steps

Making use of the safeTransferFrom() function will ensure that whoever receives the ERC721 token is able to properly support it. This protects users from losing tokens.
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels May 10, 2022
code423n4 added a commit that referenced this issue May 10, 2022
@code423n4
jayjonah8 issue #14
e746645
@outdoteth outdoteth added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label May 15, 2022
@outdoteth
Copy link
Collaborator

use safeTransferFrom to prevent stuck NFTs: #38

@outdoteth outdoteth added the duplicate This issue or pull request already exists label May 15, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@outdoteth @code423n4