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

msg.sender is unchecked in exercise(), which can cause user's NFT to be frozen #3

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

msg.sender is unchecked in exercise(), which can cause user's NFT to be frozen #3

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/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L294-L295

Vulnerability details

Impact

The msg.sender will receive the NFT when exercise() is called. However, if msg.sender is a contract address that does not support ERC721, the NFT can be frozen in the contract.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Use safeTransferFrom instead of transferFrom
@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
cccz issue #3
22e97c9
@code423n4 code423n4 mentioned this issue May 14, 2022
Open
@outdoteth
Copy link
Collaborator

use safeTransferFrom to prevent stuck NFTs: #38

@outdoteth outdoteth added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") duplicate This issue or pull request already exists labels 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