NEP-171 Vulnerable to Denial of Service

[fulldecent]

Problem

These two specifications are incompatible:

function nft_transfer If using Approval Management, contract MUST nullify approved accounts on successful transfer.

https://nomicon.io/Standards/NonFungibleToken/Core.html

and:

function nft_approve Contract MUST panic if addition would cause nft_revoke_all to exceed single-block gas limit. See below for more info.

https://nomicon.io/Standards/NonFungibleToken/ApprovalManagement.html

Why is this a problem

Because nft_transfer performs nft_revoke_all in addition to other activity, it is possible that nft_revoke_all could succeed and nft_transfer could fail.

Impact

A token owner could get into a situation where they are not able to transfer a token. The workaround is that they would need to perform nft_revoke_all before performing a transfer and everything will be okay.

But if the token owner is a smart contract, and the contract does not support calls to nft_revoke_all (which is a reasonable design decision), then it could become impossible for that token owner to transfer tokens.

Recommendations

1. Update Non-Fungible Token (NEP-171) specification. Remove the word "nullify". Add more specific language including reference to nft_revoke_all.
2. Update Non-Fungible Token Approval Management (NEP-178). Update specification for nft_approve. Specify that panic must occur not only if addition would cause nft_revoke_all to panic, but also if addition would cause nft_transfer to panic.
3. Update Non-Fungible Token Approval Management (NEP-178). Specify that the "approval ID" should be incremented. (Currently only normatively specified as "a unique approval ID". Other language "Contract MUST increment approval ID even if" is not normative on the "unique approval ID".)

[mikedotexe]

Because nft_transfer performs nft_revoke_all in addition to other activity, it is possible that nft_revoke_all could succeed and nft_transfer could fail.

I think there's another misunderstanding. In the implementation this is specifically called out, unless there's something far deeper I'm unaware of.

When a user calls nft_transfer they will hit:

    fn nft_transfer(
       …
    ) {
        …
        self.internal_transfer(&sender_id, &receiver_id, &token_id, approval_id, memo);
    }

https://github.com/near/near-sdk-rs/blob/80e8b6fe7c7ebcf700d71491f3c0ef7c7849b859/near-contract-standards/src/non_fungible_token/core/core_impl.rs#L366

then calls that function where it calls it out:

    pub fn internal_transfer(
        …
    ) -> (AccountId, Option<HashMap<AccountId, u64>>) {
        let owner_id =
            self.owner_by_id.get(token_id).unwrap_or_else(|| env::panic_str("Token not found"));

        // clear approvals, if using Approval Management extension
        // this will be rolled back by a panic if sending fails
        let approved_account_ids =
            self.approvals_by_id.as_mut().and_then(|by_id| by_id.remove(token_id));
        …

https://github.com/near/near-sdk-rs/blob/80e8b6fe7c7ebcf700d71491f3c0ef7c7849b859/near-contract-standards/src/non_fungible_token/core/core_impl.rs#L241-L244

When a panic occurs within a Rust smart contract on NEAR, the transaction is reverted along with state changes.

[fulldecent]

@mikedotexe Thank you for the references.

I agree that the nft_transfer function does clear approvals. This is coded as expected. ✅

I agree that nft_transfer will fail if there is not enough gas to clear approvals. This is coded as expected. ✅

It is possible that the amount of gas required to successfully call nft_transfer will exceed the single-block gas limit. This is bad. 💣