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

Bucket drop is the most dangerous function and should be unsafe or deprecated #1868

Open
bbarwik opened this issue Aug 6, 2024 · 0 comments

Comments

@bbarwik
Copy link
Contributor

bbarwik commented Aug 6, 2024

From my experience as auditor, the most dangerous function is "bucket.burn()".

For example, let's say we have the following function:

        pub fn repay_loan(&mut self, loan_repayment: Bucket, loan_terms: Bucket) {
            // Verify we are being sent at least the amount due
            let terms: LoanDue = loan_terms.as_non_fungible().non_fungible().data();
            assert!(
                loan_repayment.amount() >= terms.amount_due,
                "Insufficient repayment given for your loan!"
            );

            // We could also verify that the resource being repaid is of the correct kind, and give a friendly
            // error message if not. For this example we'll just let the engine handle that when we try to deposit
            self.loan_vault.put(loan_repayment);

            // We have our payment; we can now burn the transient token
            self.auth_vault
                .as_fungible()
                .authorize_with_amount(dec!(1), || loan_terms.burn());
        }

In this function, loan_terms resource address is nowhere checked so someone could create NFT compatible with LoanDue and it would work correctly because loan_terms.burn() is burning any kind of NFT.

Instead of this behavior I propose to use

self.transient_resource_manager.burn(loan_terms);

so we can always be sure that we are burning correct bucket and validation is not needed.

That's why I propose to entirely remove bucket.drop() or keep it unsafe because there are almost no use cases where it should be used without validation.

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

No branches or pull requests

1 participant