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

No tokenType validation may lead to user losing funds #71

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

No tokenType validation may lead to user losing funds #71

code423n4 opened this issue May 12, 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

Cally.sol#L158-L17

Vulnerability details

Impact

Scenario 1:

  • User creates vault for an ERC721, but mistakenly inputs an approved ERC20 address.
  • Users ERC20 balance value is equal or bigger than the intended tokenId value.
  • User mistakenly sends ERC20 to the Vault
  • People buying options from vault creator might take advantage of this by being able to purchase an ERC20 for a lower price than what it is worth depending on inputs such as dutchAuctionReserveStrike and dutchAuctionStartingStrikeIndex.

Scenario 2 (extremely unlikely):

  • User creates vault for an ERC20, but mistakenly inputs an ERC721 address.
  • Users ERC20 balance value is equal to ERC721 tokenId value.
  • People buying options from vault creator might take advantage of this by being able to purchase a ERC721 for a lower price than what it is worth depending on inputs such as dutchAuctionReserveStrike and dutchAuctionStartingStrikeIndex.

Proof of Concept

Cally.sol#L198-L200

vault.tokenType == TokenType.ERC721
  ? ERC721(vault.token).transferFrom(
      msg.sender,
      address(this),
      vault.tokenIdOrAmount
    )
  : ERC20(vault.token).safeTransferFrom(
      msg.sender,
      address(this),
      vault.tokenIdOrAmount
    );

ERC721.transferFrom may actually call a ERC20 transferFrom and vice versa.

Recommended Mitigation Steps

Dont allow user input to be the sole validator on ERC being an ERC20 or ERC721.

If TokenType.ERC20 implement extra inputs and require functions to ensure name, symbol and decimals match inputs. Same thing for TokenType.ERC721 excluding decimals and confirming inputs match name, symbol and tokenId/id.

Only allow whitelisted tokens to be used as collateral.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels May 12, 2022
code423n4 added a commit that referenced this issue May 12, 2022
@outdoteth outdoteth added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) and removed disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels May 15, 2022
@outdoteth
Copy link
Collaborator

use safeTransferFrom to prevent stuck NFTs: #38

fundamentally this is the same issue as above.

@outdoteth outdoteth added the duplicate This issue or pull request already exists label May 15, 2022
@JeeberC4 JeeberC4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jun 1, 2022
# for free to join this conversation on GitHub. Already have an account? # to 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")
Projects
None yet
Development

No branches or pull requests

3 participants