Adding locked funds may undercut current stakers rewards due to `drip` being called before totalFunds is increased

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L146-L157

Vulnerability details

Adding locked funds may undercut current stakers rewards due to drip being called before totalFunds is increased

A call to addLockedFunds() can potentially undercut staking rewards since the dripping process is triggered before the actual balance of lockedFunds is increased.

Impact

The wxETH contract works as a vault in which holders of xETH can stake their tokens to earn rewards. These rewards are implemented through a "dripping" mechanism in which amounts of xETH provided by the protocol owners (coming from different sources of revenue) are "dripped" each block.

After the drip process is started, re-adding funds to the vault can lead to an scenario in which current stakers rewards are undercut due to how the implementation of addLockedFunds() works.

https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L146-L157

146:     function addLockedFunds(uint256 amount) external onlyOwner drip {
147:         /// @dev if amount or _dripRatePerBlock is 0, revert.
148:         if (amount == 0) revert AmountZeroProvided();
149: 
150:         /// @dev transfer xETH from the user to the contract
151:         xETH.safeTransferFrom(msg.sender, address(this), amount);
152: 
153:         /// @dev add the amount to the locked funds variable
154:         lockedFunds += amount;
155: 
156:         emit LockedFundsAdded(amount, lockedFunds);
157:     }

This function implements the drip modifier which basically calls _accrueDrip() before executing the body of the function:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L222-L256

222:     function _accrueDrip() private {
223:         /// @dev if drip is disabled, no need to accrue
224:         if (!dripEnabled) return;
225: 
226:         /// @dev blockDelta is the difference between now and last accrual
227:         uint256 blockDelta = block.number - lastReport;
228: 
229:         if (blockDelta != 0) {
230:             /// @dev calculate dripAmount using blockDelta and dripRatePerBlock
231:             uint256 dripAmount = blockDelta * dripRatePerBlock;
232: 
233:             /// @dev We can only drip what we have
234:             /// @notice if the dripAmount is greater than the lockedFunds
235:             /// @notice then we set the dripAmount to the lockedFunds
236:             if (dripAmount > lockedFunds) dripAmount = lockedFunds;
237: 
238:             /// @dev unlock the dripAmount from the lockedFunds
239:             /// @notice so that it reflects the amount of xETH that is available
240:             /// @notice and the exchange rate shows that
241:             lockedFunds -= dripAmount;
242: 
243:             /// @dev set the lastReport to the current block
244:             lastReport = block.number;
245: 
246:             /// @notice if there are no remaining locked funds
247:             /// @notice the drip must be stopped.
248:             if (lockedFunds == 0) {
249:                 dripEnabled = false;
250:                 emit DripStopped();
251:             }
252: 
253:             /// @dev emit succesful drip event with dripAmount, lockedFunds and xETH balance
254:             emit Drip(dripAmount, lockedFunds, xETH.balanceOf(address(this)));
255:         }
256:     }

As we can see in the previous snippets of code, the dripping mechanism is triggered before the actual locked funds are increased, as the _accrueDrip() function is executed before the main body of addLockedFunds(). In particular, this means that line 236, which caps the drip amount to the available locked funds, is executed before line 154, which increases the funds.

This can lead to an undesired scenario where current stakers are not rewarded in full. To better visualize the issue, let's say the drip rate is set to 1, and X amount of funds are locked in block b1. Given the current conditions, funds will last for b1 + X blocks. Now suppose we are at block b2 such that b2 > b1 + X, and the protocol owners add funds to the contract by calling addLockedFunds() to fulfill the rewards. Because _accrueDrip() is executed before, dripAmount will be b2 - b1 = Y + b1 + X - b1 = Y + X being Y > 0 the difference of b2 and b1+X. This means that dripAmount > lockedFunds is true, and consequently dripAmount gets capped to lockedFunds. As a conclusion, the rewards for the Y period are not paid, even though that technically funds are being added and balance should be enough to pay these rewards.

             X                       Y
    /----------------------\ /-------------------\                         
==========================================================================>
   ^                        ^                     ^
   b1:                      b1+X                  b2
   rewards are              stakers will          funds are readded
   started with             only be rewarded      to the contract
   X amount                 upto this block

Note that this can also inadvertently stop the dripping functionality. When dripAmount is capped at lockedFunds, line 241 will set lockedFunds to zero, which means that lines 248-251 will disable the dripping mechanism.

Proof of concept

The following test demonstrates the issue. Here, the drip rate is configured to be 1e18 per block and the initial amount of locked funds is 3e18. Once 5 blocks have passed, the owner refills the contract by calling addLockedFunds(). Even though funds are enough, Alice will receive rewards for only 3 blocks when unstaking. Note that the dripping has been disabled too.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_wxETH_UndercutRewards() public {
   uint256 aliceAmount = 1e18;
   uint256 deployerAmount = 100e18;
   deal(address(xETH), alice, aliceAmount);
   deal(address(xETH), deployer, deployerAmount);

   // Deployer setups drip with 1e18 drip per block and locks 3e18 tokens
   vm.startPrank(deployer);

   xETH.approve(address(wxETH), type(uint256).max);

   wxETH.setDripRate(1e18);
   wxETH.addLockedFunds(3e18);
   wxETH.startDrip();

   vm.stopPrank();

   // Alice stakes her xETH
   vm.startPrank(alice);

   xETH.approve(address(wxETH), type(uint256).max);
   wxETH.stake(aliceAmount);

   vm.stopPrank();

   // Simulate 5 blocks ellapse
   vm.roll(block.number + 5);

   // Now deployers decides to refill contract
   vm.prank(deployer);
   wxETH.addLockedFunds(10e18);

   vm.startPrank(alice);

   // Alice decides to unstake
   wxETH.unstake(wxETH.balanceOf(alice));

   // Even though funds were available to distribute rewards for 5 blocks she only gets her initial deposit
   // plus the 3e18 from only 3 blocks
   assertEq(xETH.balanceOf(alice), 1e18 + 3e18);

   // This also disables the dripping!
   assertFalse(wxETH.dripEnabled());

   vm.stopPrank();
}

Recommendation

Remove the drip modifier from the addLockedFunds() function. The call to _accrueDrip() is not technically needed here, and removing it will solve the issue.

Assessed type

Other

[c4-judge]

kirk-baird marked the issue as primary issue

[kirk-baird]

This is a valid edge case where calling drip before adding the locked funds will result in these locked funds not being distributed to previous stakers.

I do not believe this meets the criteria for medium severity as there are no requirements for an owner, when adding more funds, to use the new funds to pay previous stakers. While it may be desirable to have locked funds pay previous stakers this is a design decision where it is valid not to action this issue.

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

kirk-baird marked the issue as grade-a

[c4-judge]

kirk-baird marked the issue as not confirmed for report

[c4-sponsor]

vaporkane marked the issue as sponsor confirmed