near / mpc

30 stars 6 forks source link

Require an attached deposit of 1 yocto for `sign` #585

Closed mikedotexe closed 3 weeks ago

mikedotexe commented 2 months ago

one yocto guard

It seems that some similar concerns exist with the MPC that we fought as a team during the authoring of the token standards. After lengthy discussion with the community and internal folks at Near Inc., the decision was made that smart contract calls to fungible and non-fungible tokens must inherently force the end user to use a full-access key on their account, for the sake of security. Now, this doesn't mean a contract author can't add nft_transfer_unsafe without this restriction, providing an alternate route. It does mean that all transfer methods covered in the standard must abide by the "one yocto guard."

The motivation was to ensure that function-call access keys couldn't be "stolen" from browser local storage (perhaps by a malicious browser extension) and used to transfer valuable tokens like USDC. It also helps deter would-be attackers, since they'll never get the key they need; a wasted effort.

Screenshot 2024-05-02 at 12 06 12 AM

In the case of chain signatures, feels like we're going to want to have that one yocto guard. In the same way the fungible token standard guarded me (as a user) from session key misuse, the MPC contract can dodge those same potential security pitfalls. We can consider the sign method on the MPC contract to be very alike to the nft_transfer in that both are the final function call before potentially transferring a large amount of value.


Not a big lift or anything, so that's good. I think we'd just strap the #[payable] macro on top the sign function, and assert one yocto first thing.

It will affect some of the examples that are currently doing multiple steps "in the background" using function-call access keys. It'll be good to flesh out the flow early, where we expect a sufficient number of web wallet users to redirect from the dApp and come back.

Discussion totally open and welcome!

Resources

DavidM-D commented 1 month ago

We did have a discussion about this internally.

The downside of this is a bit of a pain for users who are just using metatransaction relayers and don't have NEAR balances on their account. The relayers would have to operate a faucet which would top up people who want to use the relayer with tiny amounts of NEAR if they want to sign something.

We settled on putting the protections to the wallet, so it will refuse to issue a LAK for the sign method. Our reasoning was that generally accounts and wallets are pretty tightly tied together and there wallets that aren't aware of the multi-chain stuff won't have any value in their keys. Users would still have to approve a very weird LAK either way.

I'm not strongly against this proposal and perhaps an alternative is to add a second sign endpoint that requires a payment that accesses different keys post launch. WDYT?

ewiner commented 1 month ago

The relayers would have to operate a faucet which would top up people who want to use the relayer with tiny amounts of NEAR if they want to sign something.

How so? I thought the deposit on a metatransaction was paid by the relayer, not the original account.

DavidM-D commented 1 month ago

I'm almost certain that Metatransactions can only attach gas to a call not a deposit, although I can't find it specifically in the NEP.

DavidM-D commented 1 month ago

Mea culpa, you're right @ewiner. I'll check with the team but I'm in favor of adding this.