Add a variant of the Vesting wallet for updating the beneficiary #264

Amxx · 2024-05-09T09:00:59Z

Some beneficiary of vesting wallets have lost access to their keys. This PR add a variant of the VestingWallet.

The admin can upgrade the wallet to that new variant and the use the new updateBeneficiary function to update the beneficiary address to a new value.

Amxx · 2024-05-09T09:01:46Z

@ernestognw

ernestognw

contracts/vesting/VestingWalletRecovery.sol

ernestognw · 2024-05-09T09:25:34Z

contracts/vesting/VestingWalletRecovery.sol

+    function updateBeneficiary(address newBeneficiary) external onlyOwner {
+        _setBeneficiary(newBeneficiary);
+        emit BeneficiaryUpdate(newBeneficiary);
+    }


Would it make sense to add another function to accept the beneficiary change? That might be a way of avoiding an issue of changing the beneficiary to a centralized exchange wallet (as an example)

That might be a way of avoiding an issue of changing the beneficiary to a centralized exchange wallet

My understanding is that is exactly what they plan to use. Apparently, coinbase has a thing where the user as a "long lasting" address that can receive assets (including ERC20) ... but I'm not 100% sure it can be use to interract with arbitrary smart contracts.

Right, I recall someone mention the Coinbase Account, but if that's the purpose then it is okay under the assumption that this VestingWallet is more legally-restricted than code-restricted. Let's just keep this in mind, might be an important detail before making the change.

Yeah since release is permisionless we can't assume that the beneficiary wallet is able to interact with the smart contract.

ernestognw · 2024-05-09T09:27:21Z

contracts/vesting/VestingWalletRecovery.sol

+
+    function updateBeneficiary(address newBeneficiary) external onlyOwner {
+        _setBeneficiary(newBeneficiary);
+        emit BeneficiaryUpdate(newBeneficiary);


I'd say this event should be emitted inside of _setBeneficiary instead.

I wanted to minimize the changes to V1. If the storage was not private I wouldn't have touched it.

I see. I was thinking of overriding _setBeneficiary in V2:

function _setBeneficiary(address newBeneficiary) override ... { super._setBeneficiary(newBeneficiary); emit BeneficiaryUpdate(newBeneficiary); }

Would you say this is acceptable?

feels a bit too much to me. It means having two function instead of one in VestingWalletRecovery.

contract VestingWalletRecovery is VestingWalletV1 { event BeneficiaryUpdate(address newBeneficiary); function updateBeneficiary(address newBeneficiary) external onlyOwner { _setBeneficiary(newBeneficiary); } function _setBeneficiary(address newBeneficiary) internal virtual override { super._setBeneficiary(newBeneficiary); emit BeneficiaryUpdate(newBeneficiary); } }

vs

contract VestingWalletRecovery is VestingWalletV1 { event BeneficiaryUpdate(address newBeneficiary); function updateBeneficiary(address newBeneficiary) external onlyOwner { _setBeneficiary(newBeneficiary); emit BeneficiaryUpdate(newBeneficiary); } }

I agree with prioritizing fewer code and fewer changes to V1.

Co-authored-by: Ernesto García <[email protected]>

Amxx · 2024-05-13T09:36:27Z

@ernestognw @frangio

I added the "light" version, that would work as a temporary implementation. Let me know what you think. I don't feel its safer, but I like that it doesn't require any change to the V1 implementation to expose the private variable.

frangio · 2024-05-13T19:08:14Z

contracts/vesting/VestingWalletRecoveryLight.sol

+    function changeOwnerAndUpgrade(address newBeneficiary, address newImplementation) external {
+        // change ownership


Should be "change beneficiary"! Owner is a different entity.

Similarly the name of the function should be changeBeneficiaryAndUpgrade

frangio · 2024-05-13T19:08:39Z

contracts/vesting/VestingWalletRecoveryLight.sol

+    uint256[50] private __gap_3;
+    // ERC1967UpgradeUpgradeable
+    uint256[50] private __gap_4;
+    // VestingWallerV1


Suggested change

// VestingWallerV1

// VestingWalletV1

frangio · 2024-05-13T19:17:56Z

contracts/vesting/VestingWalletRecoveryLight.sol

+    bytes32 internal constant _IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
+    event Upgraded(address indexed implementation);
+
+    function changeOwnerAndUpgrade(address newBeneficiary, address newImplementation) external {


I think this should either be onlyOwner or the parameters should be immutably set in the constructor. Otherwise there is a possible error where the wallet is upgraded without calling this function and it becomes open to anyone to steal the tokens.

Yeah, considering it's supposed to be light, then setting immutables at construction sounds better

frangio · 2024-05-13T19:20:39Z

contracts/vesting/VestingWalletRecovery.sol

+
+    function updateBeneficiary(address newBeneficiary) external onlyOwner {
+        _setBeneficiary(newBeneficiary);
+        emit BeneficiaryUpdate(newBeneficiary);


I agree with prioritizing fewer code and fewer changes to V1.

frangio · 2024-05-13T19:22:03Z

contracts/vesting/VestingWalletRecovery.sol

+/**
+ * This contract is designed for recovery in case the beneficiary was lost.
+ */
+contract VestingWalletRecovery is VestingWalletV1 {


There is also VestingWalletV2. I don't know if any of the affected wallets are using V2 but if so this recovery upgrade would remove V2 features.

This seems like another reason to use the "light" recovery mode. It keeps the wallet in the same version it's at (V1 or V2).

frangio · 2024-05-13T19:24:26Z

contracts/vesting/VestingWalletRecovery.sol

+    function updateBeneficiary(address newBeneficiary) external onlyOwner {
+        _setBeneficiary(newBeneficiary);
+        emit BeneficiaryUpdate(newBeneficiary);
+    }


Yeah since release is permisionless we can't assume that the beneficiary wallet is able to interact with the smart contract.

frangio · 2024-05-13T19:26:21Z

contracts/vesting/VestingWalletRecoveryLight.sol

+ * This contract is designed for recovery in case the beneficiary was lost.
+ */
+contract VestingWalletRecoveryLight {
+    /// Storage


I think that the storage layout should be that of V2.

frangio · 2024-05-13T19:28:09Z

test/vesting/VestingWallet.recovery.test.js

+
+                this.vesting = await deployUpgradeable(
+                    hre,
+                    'VestingWallet',


This seems to be deploying "V0" of the vesting wallet:

forta-contracts/contracts/vesting/_old/vesting_wallet/VestingWalletV0.sol

Line 12 in 7d5489a

contract VestingWallet is OwnableUpgradeable, UUPSUpgradeable {

I think it should be VestingWalletV2 ?

The wallets we are targeting are using V0.

Not that the storage layout described in VestingWalletRecoveryLight is common to V0, V1 and V2.

ernestognw · 2024-05-14T00:34:35Z

test/vesting/VestingWallet.recovery.test.js

Following Fran's comments, I'd rename this file to VestingWalletV1.recovery.test.js. Consistent with the name of the underlying wallet that's being updated.

#264 (comment)

Amxx · 2024-05-14T15:14:38Z

Replaced by OpenZeppelin#1

add a variant of the Vesting wallet for updating the beneficiary

5f3cef7

Amxx requested a review from frangio May 9, 2024 09:01

up

0e49e38

Amxx temporarily deployed to development May 9, 2024 09:03 — with GitHub Actions Inactive

ernestognw reviewed May 9, 2024

View reviewed changes

Update contracts/vesting/VestingWalletRecovery.sol

2bc8040

Co-authored-by: Ernesto García <[email protected]>

Amxx temporarily deployed to development May 9, 2024 09:33 — with GitHub Actions Inactive

transitory upgrade

ea128e2

Amxx temporarily deployed to development May 13, 2024 09:33 — with GitHub Actions Inactive

up

42adfd9

Amxx temporarily deployed to development May 13, 2024 09:34 — with GitHub Actions Inactive

frangio reviewed May 13, 2024

View reviewed changes

ernestognw reviewed May 14, 2024

View reviewed changes

Amxx closed this May 14, 2024

Amxx deleted the vesting-recovery branch May 14, 2024 15:01

Amxx mentioned this pull request May 14, 2024

Add a variant of the Vesting wallet for updating the beneficiary OpenZeppelin/forta-contracts#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a variant of the Vesting wallet for updating the beneficiary #264

Add a variant of the Vesting wallet for updating the beneficiary #264

Amxx commented May 9, 2024

Amxx commented May 9, 2024

ernestognw left a comment •

edited

Loading

ernestognw May 9, 2024

Amxx May 9, 2024

ernestognw May 9, 2024 •

edited

Loading

frangio May 13, 2024

ernestognw May 9, 2024

Amxx May 9, 2024

ernestognw May 9, 2024 •

edited

Loading

Amxx May 13, 2024

frangio May 13, 2024

Amxx commented May 13, 2024

frangio May 13, 2024

ernestognw May 13, 2024

frangio May 13, 2024

frangio May 13, 2024

ernestognw May 14, 2024

frangio May 13, 2024

frangio May 13, 2024

frangio May 13, 2024

frangio May 13, 2024 •

edited

Loading

frangio May 13, 2024

Amxx May 14, 2024

ernestognw May 14, 2024

Amxx May 14, 2024

Amxx commented May 14, 2024

		function changeOwnerAndUpgrade(address newBeneficiary, address newImplementation) external {
		// change ownership

Add a variant of the Vesting wallet for updating the beneficiary #264

Add a variant of the Vesting wallet for updating the beneficiary #264

Conversation

Amxx commented May 9, 2024

Amxx commented May 9, 2024

ernestognw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernestognw May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ernestognw May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amxx commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frangio May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amxx commented May 14, 2024

ernestognw left a comment •

edited

Loading

ernestognw May 9, 2024 •

edited

Loading

ernestognw May 9, 2024 •

edited

Loading

frangio May 13, 2024 •

edited

Loading