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

[BUG] _refund function can drain the contract #13

Open
ramyhhh opened this issue Jun 5, 2022 · 5 comments
Open

[BUG] _refund function can drain the contract #13

ramyhhh opened this issue Jun 5, 2022 · 5 comments

Comments

@ramyhhh
Copy link

ramyhhh commented Jun 5, 2022

Reproduce scenario

The bug can be repreduced by following the steps:

  1. address A mints 10 tokens (sending x amount to the contract and owing 0 to 9 ids)
  2. address V also mints 10 tokens
  3. now address A can _refund for token 0 and gets the amount x transferred back
  4. again address A can _refund for token 1 and also gets the amount x transferred (out of what V sent)
  5. address A keep calling _refund as long as they have tokens in their balance and the contract has ethers.

image

This screenshot shows how I got more than the original 100 ethers in the first address draining the contract.

The problem

The problem is complex, it starts from the fact that pricePaid function is returning the whole value sent in transaction regardless of the amount of tokens minted while it should return msg.value / amount (per slot).
Also _refund is not checking against the amount refunded (which should always be 100% the same as paid price). Therefore, _refund should track amount of refunded per token and compare against the total refunded amount.

Solution

Edit ONE

Fix _safeMint to return the amount sent per token. ex:

_tokenData[updatedIndex].price = pricePaid_ / amount; // i'm not sure about all the consequences of this

Edit TWO

Currently _refund tracks token by boolean flag like:

_tokenData[tokenId].refunded = true;

It should instead track by setting the refundAmount

_tokenData[tokenId].refunded = refundAmount; //if greater than zero then this is refunded token
_ownerData[_msgSender()].refunded += refundAmount; //to make sure owner won't get more than what has been paid

and check for amount like

_owner = _ownerData[_msgSender();
if(_owner.refunded + refundAmount > _owner.paid) revert(); //this also assumes paid field is introduce

Notes

I haven't put more thoughts in the solutions, but at least bringing the issue onto the radar for now.

@byq-qoir
Copy link
Contributor

byq-qoir commented Jun 8, 2022

Thanks for raising the issue.

Originally the contract calculates the refundable amount from
pricePaid / amount
However I have changed it to pricePaid due for gas saving

On the implementation contract, you should pass the pricePaid per token each time.

I will try to reproduce the issue you have.

Thanks again :)

@byq-qoir
Copy link
Contributor

byq-qoir commented Jun 9, 2022

Hi @ramyhhh ,

Could you post the implementation contract snippet ? I'd like to see how it's call the mint function.

Will

@ramyhhh
Copy link
Author

ramyhhh commented Jun 9, 2022

Hi @0xwil
I used a very simple implementation, please check below:

contract TEST721RA is ERC721RA {
    
    uint256 private constant _mintPrice = 1 ether;

    constructor() ERC721RA("TEST", "TST",block.timestamp + (1 days)) {}

    function mint(uint256 amount) external payable {
        require(msg.value >= _mintPrice,"PAY_MINT_PRICE");
        _safeMint(msg.sender, amount, msg.value);
    }
    function withdraw() public onlyOwner {
        _withdraw(msg.sender);
    }
    function refund(uint256 tokenId) public {
        _refund(msg.sender,tokenId);
    }
    function tvl() external view returns (uint256) {
        return address(this).balance;
    }
}

@byq-qoir
Copy link
Contributor

byq-qoir commented Jun 9, 2022

Hi @ramyhhh ,

I see the problem.

function mint(uint256 amount) external payable {
        require(msg.value >= _mintPrice,"PAY_MINT_PRICE");
        _safeMint(msg.sender, amount, msg.value);  // Instead of passing msg.value, you should pass the price per token.
}

Try the following:

function mint(uint256 amount) external payable {
        require(msg.value >= _mintPrice * amount,"PAY_MINT_PRICE");
        _safeMint(msg.sender, amount, _mintPrice);
       // in case if the msg.value is greater than _mintPrice * amount, 
       // you can also transfer the remainder back the the msg.sender,
       // so there is no point to pass the msg.value to _safeMint()
 }

@ramyhhh
Copy link
Author

ramyhhh commented Jun 9, 2022

I see your point, however I still think that it's pretty risky not to check the refund value and protect against over refund.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants