From 4b2cde9ce30401dcaa8ccb188da7df99268cb0e8 Mon Sep 17 00:00:00 2001 From: Shuhui Luo <107524008+shuhuiluo@users.noreply.github.com> Date: Wed, 16 Oct 2024 16:52:09 -0400 Subject: [PATCH] fix(contract): remove token delegation threshold and fix lock (#1228) Eliminated the token threshold mechanism from the `River` contract, including the removal of the `MIN_TOKEN_THRESHOLD` variable and the `setTokenThreshold` function. Updated related tests to reflect these changes, ensuring consistent functionality throughout the contract. Replaced `IntrospectionBase` and `LockBase` with `IntrospectionFacet` and `LockFacet` for better modularization. Removed redundant lock methods and corrected minor typos in comments for improved code clarity and maintainability. Update logic in `_setDelegators` to handle edge cases for delegate assignment more efficiently. Simplify `River._delegate` by reusing the current delegate storage. Fix delegation lock and add tests. --- .../votes/enumerable/VotesEnumerable.sol | 16 ++- contracts/src/tokens/lock/ILock.sol | 3 +- contracts/src/tokens/lock/LockBase.sol | 31 +++--- contracts/src/tokens/river/base/River.sol | 84 ++++---------- contracts/src/tokens/river/mainnet/River.sol | 4 +- .../test/river/token/base/RiverBase.t.sol | 103 ++++++++++++------ 6 files changed, 114 insertions(+), 127 deletions(-) diff --git a/contracts/src/diamond/facets/governance/votes/enumerable/VotesEnumerable.sol b/contracts/src/diamond/facets/governance/votes/enumerable/VotesEnumerable.sol index 0611cfc15..d611e7430 100644 --- a/contracts/src/diamond/facets/governance/votes/enumerable/VotesEnumerable.sol +++ b/contracts/src/diamond/facets/governance/votes/enumerable/VotesEnumerable.sol @@ -36,12 +36,18 @@ abstract contract VotesEnumerable is IVotesEnumerable { ) internal virtual { VotesEnumerableStorage.Layout storage ds = VotesEnumerableStorage.layout(); - ds.delegators.remove(account); - ds.delegatorsByDelegatee[currentDelegatee].remove(account); - - // if the delegatee is not address(0) then add the account and is not already a delegator then add it - if (newDelegatee != address(0)) { + // if the current delegatee is address(0) then add the account + if (currentDelegatee == address(0)) { ds.delegators.add(account); + } else { + ds.delegatorsByDelegatee[currentDelegatee].remove(account); + } + + if (newDelegatee == address(0)) { + ds.delegators.remove(account); + ds.delegationTimeForDelegator[account] = 0; + } else { + // if the new delegatee is not address(0) then add the account ds.delegatorsByDelegatee[newDelegatee].add(account); ds.delegationTimeForDelegator[account] = block.timestamp; } diff --git a/contracts/src/tokens/lock/ILock.sol b/contracts/src/tokens/lock/ILock.sol index 983cb83ef..4b2246174 100644 --- a/contracts/src/tokens/lock/ILock.sol +++ b/contracts/src/tokens/lock/ILock.sol @@ -15,8 +15,7 @@ interface ILockBase { event LockUpdated( address indexed caller, bool indexed enabled, - uint256 cooldown, - uint256 timestamp + uint256 cooldown ); } diff --git a/contracts/src/tokens/lock/LockBase.sol b/contracts/src/tokens/lock/LockBase.sol index 919ec4705..4ca1a6cf3 100644 --- a/contracts/src/tokens/lock/LockBase.sol +++ b/contracts/src/tokens/lock/LockBase.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.23; import {ILockBase} from "./ILock.sol"; // libraries +import {CustomRevert} from "contracts/src/utils/libraries/CustomRevert.sol"; import {LockStorage} from "./LockStorage.sol"; abstract contract LockBase is ILockBase { @@ -24,31 +25,25 @@ abstract contract LockBase is ILockBase { function _enableLock(address caller) internal { LockStorage.Layout storage ds = LockStorage.layout(); - if (ds.enabledByAddress[caller]) { - revert LockAlreadyEnabled(); - } + if (ds.enabledByAddress[caller]) + CustomRevert.revertWith(LockAlreadyEnabled.selector); ds.enabledByAddress[caller] = true; - emit LockUpdated(caller, true, 0, block.timestamp); + emit LockUpdated(caller, true, 0); } function _disableLock(address caller) internal { LockStorage.Layout storage ds = LockStorage.layout(); - if (ds.enabledByAddress[caller] == false) { - revert LockAlreadyDisabled(); - } + if (!ds.enabledByAddress[caller]) + CustomRevert.revertWith(LockAlreadyDisabled.selector); + uint256 cooldown = block.timestamp + ds.defaultCooldown; ds.enabledByAddress[caller] = false; - ds.cooldownByAddress[caller] = block.timestamp + ds.defaultCooldown; - - emit LockUpdated( - caller, - false, - block.timestamp + ds.defaultCooldown, - block.timestamp - ); + ds.cooldownByAddress[caller] = cooldown; + + emit LockUpdated(caller, false, cooldown); } function _lockCooldown(address caller) internal view returns (uint256) { @@ -58,9 +53,9 @@ abstract contract LockBase is ILockBase { function _lockEnabled(address caller) internal view returns (bool) { LockStorage.Layout storage ds = LockStorage.layout(); - return - ds.enabledByAddress[caller] == true || - block.timestamp < ds.cooldownByAddress[caller]; + if (ds.enabledByAddress[caller]) return true; + + return block.timestamp < ds.cooldownByAddress[caller]; } function _canLock() internal view virtual returns (bool); diff --git a/contracts/src/tokens/river/base/River.sol b/contracts/src/tokens/river/base/River.sol index eef2dea8d..f2a1d02a7 100644 --- a/contracts/src/tokens/river/base/River.sol +++ b/contracts/src/tokens/river/base/River.sol @@ -22,27 +22,25 @@ import {ERC20Votes} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Vo import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {VotesEnumerable} from "contracts/src/diamond/facets/governance/votes/enumerable/VotesEnumerable.sol"; -import {IntrospectionBase} from "contracts/src/diamond/facets/introspection/IntrospectionBase.sol"; -import {LockBase} from "contracts/src/tokens/lock/LockBase.sol"; +import {IntrospectionFacet} from "contracts/src/diamond/facets/introspection/IntrospectionFacet.sol"; +import {LockFacet} from "contracts/src/tokens/lock/LockFacet.sol"; contract River is IOptimismMintableERC20, ILegacyMintableERC20, ISemver, - ILock, + Ownable, ERC20Permit, ERC20Votes, - Ownable, VotesEnumerable, - IntrospectionBase, - LockBase + LockFacet, + IntrospectionFacet { // ============================================================= // Errors // ============================================================= error River__TransferLockEnabled(); error River__DelegateeSameAsCurrent(); - error River__InvalidTokenAmount(); // ============================================================= // Events @@ -62,11 +60,6 @@ contract River is /// @notice Address of the StandardBridge on this network. address public immutable BRIDGE; - // ============================================================= - // Variables - // ============================================================= - uint256 public MIN_TOKEN_THRESHOLD = 10 ether; - // ============================================================= // Modifiers // ============================================================= @@ -150,7 +143,8 @@ contract River is // ============================================================= // Votes // ============================================================= - /// @notice Clock used for flagging checkpoints, overriden to implement timestamp based + + /// @notice Clock used for flagging checkpoints, overridden to implement timestamp based /// checkpoints (and voting). function clock() public view override returns (uint48) { return uint48(block.timestamp); @@ -163,7 +157,7 @@ contract River is function nonces( address owner - ) public view virtual override(ERC20Permit, Nonces) returns (uint256) { + ) public view override(ERC20Permit, Nonces) returns (uint256) { return super.nonces(owner); } @@ -172,91 +166,51 @@ contract River is // ============================================================= /// @inheritdoc ILock - function isLockEnabled(address account) external view virtual returns (bool) { - return _lockEnabled(account); - } - - function lockCooldown( - address account - ) external view virtual returns (uint256) { - return _lockCooldown(account); - } - - /// @inheritdoc ILock - function enableLock(address account) external virtual onlyOwner {} + function enableLock(address account) external override onlyOwner {} /// @inheritdoc ILock - function disableLock(address account) external virtual onlyOwner {} + function disableLock(address account) external override onlyOwner {} /// @inheritdoc ILock - function setLockCooldown(uint256 cooldown) external virtual onlyOwner { + function setLockCooldown(uint256 cooldown) external override onlyOwner { _setDefaultCooldown(cooldown); } - // ============================================================= - // IERC165 - // ============================================================= - - /// @inheritdoc IERC165 - function supportsInterface(bytes4 interfaceId) public view returns (bool) { - return _supportsInterface(interfaceId); - } - - // ============================================================= - // Token - // ============================================================= - function setTokenThreshold(uint256 threshold) external onlyOwner { - if (threshold > totalSupply()) revert River__InvalidTokenAmount(); - MIN_TOKEN_THRESHOLD = threshold; - emit TokenThresholdSet(threshold); - } - // ============================================================= // Hooks // ============================================================= + function _update( address from, address to, uint256 value - ) internal virtual override(ERC20, ERC20Votes) { + ) internal override(ERC20, ERC20Votes) { if (from != address(0) && _lockEnabled(from)) { - // allow transfering at minting time + // allow transferring at minting time revert River__TransferLockEnabled(); } super._update(from, to, value); } - function _getVotingUnits( - address account - ) internal view override returns (uint256) { - return balanceOf(account); - } - /// @dev Hook that gets called before any external enable and disable lock function function _canLock() internal view override returns (bool) { return msg.sender == owner(); } - function _delegate( - address account, - address delegatee - ) internal virtual override { - // revert if the delegatee is the same as the current delegatee - if (delegates(account) == delegatee) revert River__DelegateeSameAsCurrent(); + function _delegate(address account, address delegatee) internal override { + address currentDelegatee = delegates(account); - // revert if the balance is below the threshold - if (balanceOf(account) < MIN_TOKEN_THRESHOLD) - revert River__InvalidTokenAmount(); + // revert if the delegatee is the same as the current delegatee + if (currentDelegatee == delegatee) revert River__DelegateeSameAsCurrent(); // if the delegatee is the zero address, initialize disable lock if (delegatee == address(0)) { _disableLock(account); } else { - if (!_lockEnabled(account)) _enableLock(account); + _enableLock(account); } - address currentDelegatee = delegates(account); super._delegate(account, delegatee); _setDelegators(account, delegatee, currentDelegatee); diff --git a/contracts/src/tokens/river/mainnet/River.sol b/contracts/src/tokens/river/mainnet/River.sol index b583dd35d..8abeef5f7 100644 --- a/contracts/src/tokens/river/mainnet/River.sol +++ b/contracts/src/tokens/river/mainnet/River.sol @@ -130,7 +130,7 @@ contract River is uint256 value ) internal virtual override(ERC20, ERC20Votes) { if (from != address(0) && _lockEnabled(from)) { - // allow transfering at minting time + // allow transferring at minting time revert River__TransferLockEnabled(); } super._update(from, to, value); @@ -170,7 +170,7 @@ contract River is /// @dev Do not allow disabling lock without delegating function disableLock(address account) external override onlyAllowed {} - /// @notice Clock used for flagging checkpoints, overriden to implement timestamp based + /// @notice Clock used for flagging checkpoints, overridden to implement timestamp based /// checkpoints (and voting). function clock() public view override returns (uint48) { return uint48(block.timestamp); diff --git a/contracts/test/river/token/base/RiverBase.t.sol b/contracts/test/river/token/base/RiverBase.t.sol index e8f3532be..cf7d4dfc6 100644 --- a/contracts/test/river/token/base/RiverBase.t.sol +++ b/contracts/test/river/token/base/RiverBase.t.sol @@ -21,13 +21,13 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { bytes32 private constant _PERMIT_TYPEHASH = 0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9; - River riverFacet; - uint256 stakeRequirement; + address internal ALICE = makeAddr("ALICE"); + address internal BOB = makeAddr("BOB"); + River internal riverFacet; function setUp() public override { super.setUp(); riverFacet = River(riverToken); - stakeRequirement = riverFacet.MIN_TOKEN_THRESHOLD(); } function test_init() external view { @@ -41,19 +41,23 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { modifier givenCallerHasBridgedTokens(address caller, uint256 amount) { vm.assume(caller != address(0)); - amount = bound(amount, stakeRequirement, type(uint208).max); + amount = bound(amount, 0, type(uint208).max); vm.prank(bridge); riverFacet.mint(caller, amount); _; } + function test_allowance() public { + test_fuzz_allowance(ALICE, 1 ether, BOB); + } + // Permit and Permit with Signature function test_fuzz_allowance( address alice, uint256 amount, address bob - ) external givenCallerHasBridgedTokens(alice, amount) { + ) public givenCallerHasBridgedTokens(alice, amount) { vm.assume(bob != address(0)); assertEq(riverFacet.allowance(alice, bob), 0); @@ -64,16 +68,16 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { assertEq(riverFacet.allowance(alice, bob), amount); } + function test_permit() public { + test_fuzz_permit(makeAccount("ALICE").key, 1 ether, BOB); + } + function test_fuzz_permit( uint256 alicePrivateKey, uint256 amount, address bob - ) external { - alicePrivateKey = bound( - alicePrivateKey, - 1, - 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140 - ); + ) public { + alicePrivateKey = boundPrivateKey(alicePrivateKey); vm.assume(bob != address(0)); amount = bound(amount, 1, type(uint208).max); @@ -101,16 +105,12 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { assertEq(riverFacet.allowance(alice, bob), amount); } - function test_fuzz_revertWhen_permitDeadlineExpired( + function test_fuzz_permit_revertWhen_deadlineExpired( uint256 alicePrivateKey, uint256 amount, address bob ) external { - alicePrivateKey = bound( - alicePrivateKey, - 1, - 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140 - ); + alicePrivateKey = boundPrivateKey(alicePrivateKey); vm.assume(bob != address(0)); amount = bound(amount, 1, type(uint208).max); @@ -157,40 +157,44 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { _; } - function test_fuzz_revertWhen_delegateToZeroAddress( - address alice, - uint256 amount - ) external givenCallerHasBridgedTokens(alice, amount) { + function test_fuzz_delegate_revertWhen_delegateToZeroAddress( + address alice + ) external { vm.prank(alice); vm.expectRevert(River.River__DelegateeSameAsCurrent.selector); riverFacet.delegate(address(0)); assertEq(riverFacet.delegates(alice), address(0)); } - // Locking - function test_fuzz_enableLock( + function test_delegate_enableLock() public { + test_fuzz_delegate_enableLock(ALICE, 1 ether); + } + + function test_fuzz_delegate_enableLock( address alice, uint256 amount ) - external + public givenCallerHasBridgedTokens(alice, amount) whenCallerDelegatesToASpace(alice) { assertEq(riverFacet.isLockEnabled(alice), true); + vm.expectEmit(riverToken); + emit LockUpdated(alice, false, block.timestamp + 30 days); + vm.prank(alice); riverFacet.delegate(address(0)); assertEq(riverFacet.isLockEnabled(alice), true); - uint256 lockCooldown = riverFacet.lockCooldown(alice); - - vm.warp(block.timestamp + lockCooldown + 1); + uint256 cd = riverFacet.lockCooldown(alice); + vm.warp(cd); assertEq(riverFacet.isLockEnabled(alice), false); } - function test_fuzz_revertWhen_transferringWhileLockEnabled( + function test_fuzz_transfer_revertWhen_lockEnabled( address alice, uint256 amount, address bob @@ -205,7 +209,37 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { riverFacet.transfer(bob, amount); } - function test_fuzz_delegateVotes_isCorrect( + function test_fuzz_transfer_revertWhen_delegating( + address alice, + uint256 amount, + address bob + ) + external + givenCallerHasBridgedTokens(alice, amount) + whenCallerDelegatesToASpace(alice) + { + amount = bound(amount, 0, type(uint208).max); + vm.assume(bob != address(0)); + + vm.startPrank(alice); + riverFacet.delegate(address(0)); + + assertEq(riverFacet.isLockEnabled(alice), true); + + riverFacet.delegate(space); + + uint256 cd = riverFacet.lockCooldown(alice); + vm.warp(cd); + + vm.expectRevert(River.River__TransferLockEnabled.selector); + riverFacet.transfer(bob, amount); + } + + function test_transfer_delegateVotesIsCorrect() public { + test_fuzz_transfer_delegateVotesIsCorrect(ALICE, 1 ether, BOB, 1 ether); + } + + function test_fuzz_transfer_delegateVotesIsCorrect( address alice, uint256 amountA, address bob, @@ -215,8 +249,8 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { vm.assume(alice != address(0)); vm.assume(bob != address(0)); - amountA = bound(amountA, 1, type(uint208).max - stakeRequirement); - amountB = bound(amountB, stakeRequirement, type(uint208).max - amountA); + amountA = bound(amountA, 1, type(uint208).max - 1); + amountB = bound(amountB, 1, type(uint208).max - amountA); vm.prank(bridge); riverFacet.mint(alice, amountA); @@ -224,8 +258,9 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { vm.prank(bridge); riverFacet.mint(bob, amountB); - vm.expectEmit(); + vm.expectEmit(riverToken); emit IVotes.DelegateVotesChanged(space, 0, amountB); + emit LockUpdated(bob, true, 0); vm.prank(bob); riverFacet.delegate(space); @@ -238,14 +273,12 @@ contract RiverBaseTest is BaseSetup, ILockBase, IOwnableBase { ); assertEq(riverFacet.getVotes(space), amountB); - vm.expectEmit(); + vm.expectEmit(riverToken); emit IVotes.DelegateVotesChanged(space, amountB, amountA + amountB); vm.prank(alice); riverFacet.transfer(bob, amountA); - vm.warp(timestamp + 10); - assertEq(riverFacet.getVotes(space), amountA + amountB); }