Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Users could accidentally burn() and lose fund #331

Closed
code423n4 opened this issue Oct 25, 2022 · 2 comments
Closed

Users could accidentally burn() and lose fund #331

code423n4 opened this issue Oct 25, 2022 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working invalid This doesn't seem right resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) responded The Holograph team has reviewed and responded

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/HolographERC721.sol#L387-L397
https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/HolographERC20.sol#L337-L345

Vulnerability details

Impact

If users accidentally burn() the token, the fund will be lost. There does not seem any reason for the users to call the public burn() function, only by mistake calling the method to lose self fund.

Proof of Concept

// contracts/enforcer/HolographERC721.sol
  function burn(uint256 tokenId) external {
    require(_isApproved(msg.sender, tokenId), "ERC721: not approved sender");
    address wallet = _tokenOwner[tokenId];
    if (_isEventRegistered(HolographERC721Event.beforeBurn)) {
      require(SourceERC721().beforeBurn(wallet, tokenId));
    }
    _burn(wallet, tokenId);
    if (_isEventRegistered(HolographERC721Event.afterBurn)) {
      require(SourceERC721().afterBurn(wallet, tokenId));
    }
  }

// contracts/enforcer/HolographERC20.sol
  function burn(uint256 amount) public {
    if (_isEventRegistered(HolographERC20Event.beforeBurn)) {
      require(SourceERC20().beforeBurn(msg.sender, amount));
    }
    _burn(msg.sender, amount);
    if (_isEventRegistered(HolographERC20Event.afterBurn)) {
      require(SourceERC20().afterBurn(msg.sender, amount));
    }
  }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Remove the public burn() functions in ERC721 and ERC20.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Oct 25, 2022
code423n4 added a commit that referenced this issue Oct 25, 2022
@gzeoneth gzeoneth added sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue invalid This doesn't seem right and removed sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue labels Oct 31, 2022
@gzeoneth
Copy link
Member

There are a lot of way that user can accidentally burn their fund, risk is very low.

@alexanderattar
Copy link

It is the users responsibility to not burn their tokens

@alexanderattar alexanderattar added the responded The Holograph team has reviewed and responded label Nov 8, 2022
@alexanderattar alexanderattar added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working invalid This doesn't seem right resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) responded The Holograph team has reviewed and responded
Projects
None yet
Development

No branches or pull requests

3 participants