Merge pull request #14 from euler-xyz/review-fix

Review fix

README.md

@@ -118,13 +118,14 @@ Unlike other permissioned distributors based on the billion-dollar algorithm, Re
 1. **Epoch duration may not be shorter than 7 days**: This limitation is in place to ensure the stability and efficiency of the distribution system. The longer the epoch, the more gas efficient the distribution is.
 2. **New distribution scheme may start at most 5 epochs ahead and be at most 25 epochs long**: This limitation is in place not to register distribution too far in the future and lasting for too long.
 3. **A user may have at most 5 rewards enabled at a time for a given rewarded token**: This limitation is in place to prevent users from enabling an excessive number of rewards, which could lead to increased gas costs and potential system instability.
-4. **During its lifetime, a distributor may distribute at most `type(uint144).max / 1e12` units of a reward token per rewarded token**: This limitation is in place not to allow accumulator overflow.
-5. **If nobody earns rewards at the moment (i.e. nobody staked/deposited yet), they're being virtually accrued by address(0) and may be claimed by anyone**: This feature is designed to prevent reward tokens from being lost when nobody earns them at the moment. However, it also means that unclaimed rewards could potentially be claimed by anyone.
-6. **Distributor contracts do not have an owner or admin meaning that none of the assets can be directly recovered from them**: This feature is required for the system to work in a permissionless manner. However, it also means that if a mistake is made in the distribution of rewards, the assets cannot be directly recovered from the distributor contracts.
+4. **A user may have at most `type(uint112).max` unclaimed reward tokens per rewarded token**: This limitation is in place to efficiently pack variables in storage, optimizing for gas costs and contract performance.
+5. **During its lifetime, a distributor may distribute at most `type(uint144).max / 1e12` units of a reward token per rewarded token**: This limitation is in place not to allow accumulator overflow.
+6. **If nobody earns rewards at the moment (i.e. nobody staked/deposited yet), they're being virtually accrued by address(0) and may be claimed by anyone**: This feature is designed to prevent reward tokens from being lost when nobody earns them at the moment. However, it also means that unclaimed rewards could potentially be claimed by anyone.
+7. **Distributor contracts do not have an owner or admin meaning that none of the assets can be directly recovered from them**: This feature is required for the system to work in a permissionless manner. However, it also means that if a mistake is made in the distribution of rewards, the assets cannot be directly recovered from the distributor contracts.
 
 ## Install
 
-To install Ethereum Vault Connector in a [Foundry](https://github.com/foundry-rs/foundry) project:
+To install Reward Streams in a [Foundry](https://github.com/foundry-rs/foundry) project:
 
 ```sh
 forge install euler-xyz/reward-streams

src/BaseRewardStreams.sol

@@ -443,6 +443,22 @@ abstract contract BaseRewardStreams is IRewardStreams, EVCUtil, ReentrancyGuard
         }
     }
 
+    /// @notice Transfers a specified amount of a token to a given address.
+    /// @dev This function uses `IERC20.safeTransfer` to move tokens.
+    /// @dev This function reverts if the recipient is zero address or is a known non-owner EVC account.
+    /// @param token The ERC20 token to transfer.
+    /// @param to The address to transfer the tokens to.
+    /// @param amount The amount of tokens to transfer.
+    function pushToken(IERC20 token, address to, uint256 amount) internal {
+        address owner = evc.getAccountOwner(to);
+
+        if (to == address(0) || (owner != address(0) && owner != to)) {
+            revert InvalidRecipient();
+        }
+
+        IERC20(token).safeTransfer(to, amount);
+    }
+
     /// @notice Returns the reward token amount for an epoch, given a pre-computed distribution storage pointer.
     /// @param distributionStorage Pre-computed distribution storage pointer.
     /// @param epoch The epoch to get the reward token amount for.
@@ -478,14 +494,13 @@ abstract contract BaseRewardStreams is IRewardStreams, EVCUtil, ReentrancyGuard
 
     /// @notice Claims the earned reward for a specific account, rewarded token, and reward token, and transfers it to
     /// the recipient.
-    /// @dev If recipient is address(0) or there is no reward to claim, this function does nothing.
+    /// @dev This function reverts if the recipient is zero address or is a known non-owner EVC account.
+    /// @dev If there is no reward to claim, this function does nothing.
     /// @param account The address of the account claiming the reward.
     /// @param rewarded The address of the rewarded token.
     /// @param reward The address of the reward token.
     /// @param recipient The address to which the claimed reward will be transferred.
     function claim(address account, address rewarded, address reward, address recipient) internal virtual {
-        if (recipient == address(0)) revert InvalidRecipient();
-
         EarnStorage storage accountEarned = accounts[account][rewarded].earned[reward];
         uint128 amount = accountEarned.claimable;
 
@@ -501,7 +516,7 @@ abstract contract BaseRewardStreams is IRewardStreams, EVCUtil, ReentrancyGuard
             distributionStorage.totalClaimed = newTotalClaimed;
             accountEarned.claimable = 0;
 
-            IERC20(reward).safeTransfer(recipient, amount);
+            pushToken(IERC20(reward), recipient, amount);
             emit RewardClaimed(account, rewarded, reward, amount);
         }
     }

src/StakingRewardStreams.sol

@@ -65,6 +65,7 @@ contract StakingRewardStreams is BaseRewardStreams, IStakingRewardStreams {
     }
 
     /// @notice Allows a user to unstake rewarded tokens.
+    /// @dev This function reverts if the recipient is zero address or is a known non-owner EVC account.
     /// @dev If the amount is max, the entire balance of the user is unstaked.
     /// @param rewarded The address of the rewarded token.
     /// @param recipient The address to receive the unstaked tokens.
@@ -109,7 +110,7 @@ contract StakingRewardStreams is BaseRewardStreams, IStakingRewardStreams {
 
         accountStorage.balance = currentAccountBalance - amount;
 
-        IERC20(rewarded).safeTransfer(recipient, amount);
+        pushToken(IERC20(rewarded), recipient, amount);
 
         emit Unstaked(msgSender, rewarded, amount);
     }

test/unit/Scenarios.t.sol

@@ -8,6 +8,7 @@ import "../harness/StakingRewardStreamsHarness.sol";
 import "../harness/TrackingRewardStreamsHarness.sol";
 import {MockERC20, MockERC20BalanceForwarder} from "../utils/MockERC20.sol";
 import {MockController} from "../utils/MockController.sol";
+import {boundAddr} from "../utils/TestUtils.sol";
 
 contract ScenarioTest is Test {
     address internal PARTICIPANT_1;
@@ -2290,22 +2291,68 @@ contract ScenarioTest is Test {
         address _receiver,
         bool _forfeitRecentReward
     ) external {
+        _rewarded = boundAddr(_rewarded);
+        _reward = boundAddr(_reward);
+        _receiver = boundAddr(_receiver);
         vm.assume(_receiver != address(0));
 
+        vm.etch(_reward, address(reward).code);
+        MockERC20(_reward).mint(address(stakingDistributor), 100e18);
+        MockERC20(_reward).mint(address(trackingDistributor), 100e18);
+        stakingDistributor.setDistributionTotals(_rewarded, _reward, 0, 100e18, 0);
+        trackingDistributor.setDistributionTotals(_rewarded, _reward, 0, 100e18, 0);
+
+        stakingDistributor.setAccountEarnedData(address(this), _rewarded, _reward, BaseRewardStreams.EarnStorage(1, 0));
         vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
         stakingDistributor.claimReward(_rewarded, _reward, address(0), _forfeitRecentReward);
         stakingDistributor.claimReward(_rewarded, _reward, _receiver, _forfeitRecentReward);
 
+        trackingDistributor.setAccountEarnedData(address(this), _rewarded, _reward, BaseRewardStreams.EarnStorage(1, 0));
         vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
         trackingDistributor.claimReward(_rewarded, _reward, address(0), _forfeitRecentReward);
         trackingDistributor.claimReward(_rewarded, _reward, _receiver, _forfeitRecentReward);
 
+        stakingDistributor.setAccountEarnedData(address(0), _rewarded, _reward, BaseRewardStreams.EarnStorage(1, 0));
         vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
         stakingDistributor.claimSpilloverReward(_rewarded, _reward, address(0));
         stakingDistributor.claimSpilloverReward(_rewarded, _reward, _receiver);
 
+        trackingDistributor.setAccountEarnedData(address(0), _rewarded, _reward, BaseRewardStreams.EarnStorage(1, 0));
         vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
         trackingDistributor.claimSpilloverReward(_rewarded, _reward, address(0));
         trackingDistributor.claimSpilloverReward(_rewarded, _reward, _receiver);
+
+        // register the receiver as the owner on the EVC
+        assertEq(evc.getAccountOwner(_receiver), address(0));
+        vm.prank(_receiver);
+        evc.call(address(0), _receiver, 0, "");
+        assertEq(evc.getAccountOwner(_receiver), _receiver);
+
+        for (uint160 i = 0; i < 256; ++i) {
+            address __receiver = address(uint160(_receiver) ^ i);
+
+            // if known non-owner is the recipient, revert
+            stakingDistributor.setAccountEarnedData(
+                address(this), _rewarded, _reward, BaseRewardStreams.EarnStorage(1, 0)
+            );
+            if (i != 0) vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
+            stakingDistributor.claimReward(_rewarded, _reward, __receiver, _forfeitRecentReward);
+
+            trackingDistributor.setAccountEarnedData(
+                address(this), _rewarded, _reward, BaseRewardStreams.EarnStorage(1, 0)
+            );
+            if (i != 0) vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
+            trackingDistributor.claimReward(_rewarded, _reward, __receiver, _forfeitRecentReward);
+
+            stakingDistributor.setAccountEarnedData(address(0), _rewarded, _reward, BaseRewardStreams.EarnStorage(1, 0));
+            if (i != 0) vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
+            stakingDistributor.claimSpilloverReward(_rewarded, _reward, __receiver);
+
+            trackingDistributor.setAccountEarnedData(
+                address(0), _rewarded, _reward, BaseRewardStreams.EarnStorage(1, 0)
+            );
+            if (i != 0) vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
+            trackingDistributor.claimSpilloverReward(_rewarded, _reward, __receiver);
+        }
     }
 }

test/unit/Staking.t.sol

@@ -82,6 +82,36 @@ contract StakingTest is Test {
         // stake malicious
         vm.expectRevert(BaseRewardStreams.InvalidAmount.selector);
         distributor.stake(rewardedMalicious, amount);
+        vm.stopPrank();
+
+        // stake max from recipient
+        vm.startPrank(recipient);
+        MockERC20(rewarded).approve(address(distributor), type(uint256).max);
+        distributor.stake(rewarded, type(uint256).max);
+
+        // unstake to zero address
+        vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
+        distributor.unstake(rewarded, type(uint256).max, address(0), false);
+
+        // register the receiver as the owner on the EVC
+        assertEq(evc.getAccountOwner(recipient), address(0));
+        evc.call(address(0), recipient, 0, "");
+        assertEq(evc.getAccountOwner(recipient), recipient);
+
+        for (uint160 i = 1; i < 256; ++i) {
+            address _recipient = address(uint160(recipient) ^ i);
+
+            // if known non-owner is the recipient, revert
+            vm.expectRevert(BaseRewardStreams.InvalidRecipient.selector);
+            distributor.unstake(rewarded, type(uint256).max, _recipient, false);
+        }
+
+        // but if owner is the recipient, it should work
+        preBalanceRecipient = MockERC20(rewarded).balanceOf(recipient);
+        preBalanceDistributor = MockERC20(rewarded).balanceOf(address(distributor));
+        distributor.unstake(rewarded, type(uint256).max, recipient, false);
+        assertEq(MockERC20(rewarded).balanceOf(recipient), preBalanceRecipient + preBalanceDistributor);
+        assertEq(MockERC20(rewarded).balanceOf(address(distributor)), 0);
 
         vm.stopPrank();
     }