0x73696d616f - Depositing to another receiver othan than `msg.sender` will lead to stuck funds by increasing `avgStart` without claiming

[sherlock-admin2]

0x73696d616f

High

Depositing to another receiver othan than msg.sender will lead to stuck funds by increasing avgStart without claiming

Summary

StakedEXA::_update() is called when minting and if the time staked is smaller than the reference time, it calls _claim(reward), which claims the rewards for themsg.sender. However, the mint may be to someone else other thanmsg.sender, soavgStart[to]` will increase without first claiming, which will lead to loss of rewards for the receiver of the new deposit.

Root Cause

In StakedEXA.sol::146 on claim(reward), it claims to the msg.sender, but the deposit is done to the to address, which may be different.

Internal pre-conditions

1. Anyone must do a deposit to a different address.

External pre-conditions

None.

Attack Path

1. User has some StakedEXA from previous deposits.
2. Another user deposits/mints to user 1, which increases avgStart[user1], but does not claim first for user1, making the funds stuck.

Impact

The funds are stuck for much longer than they are supposed to. The duration is 24 weeks in the tests, so users can have to wait up to 24 weeks more if they do a really big deposit while having fully claimable rewards. Additionally, in case they decide to withdraw after depositing, before the rewards vest again, they will lose these funds to savings.

PoC

Paste the following test in StakedEXA.t.sol to confirm that address(this) deposits more without claiming.

function test_POC_increaseBalance_withoutClaim() external {
  uint256 assets = 1e18;
  uint256 time = minTime;

  exa.mint(address(this), assets);
  stEXA.deposit(assets, address(this));

  uint256 initTime = block.timestamp * 1e18;
  skip(time + 1);
  uint256 finalTime = block.timestamp * 1e18;

  assertEq(rA.balanceOf(address(this)), 0);
  assertEq(stEXA.avgStart(address(this)), initTime);
  assertEq(stEXA.rawClaimable(rA, address(this), assets), 20833367779982251207);
  assertEq(stEXA.earned(rA, address(this), assets), 41666735559964287164);

  address anotherAccount = makeAddr("AnotherAccount");
  exa.mint(anotherAccount, assets);
  vm.startPrank(anotherAccount);
  exa.approve(address(stEXA), assets);
  stEXA.deposit(assets, address(this));
  vm.stopPrank();

  uint256 newAssets = 2*assets;

  assertEq(rA.balanceOf(address(this)), 0);
  assertEq(stEXA.avgStart(address(this)), (initTime + finalTime) / 2);
  assertEq(stEXA.rawClaimable(rA, address(this), newAssets), 0);
  assertEq(stEXA.earned(rA, address(this), newAssets), 41666735559964287164);
}

Mitigation

Claiming should be made to the receiver of the funds, to. The _claim() function should be adapted to receiver an account argument.

[santiellena]

Escalate.

This issue should be Low/Informational.

In order to "exploit" this vulnerability, the attacker has to donate a sufficient amount to the victim to move the avgStart parameter enough. The inability of the victim to withdraw will be temporal and not permanent because as time passes, the penalty decreases, so the issue is incorrect when states that will lead to stuck funds.

Additionally, as this issue is about causing temporal unavailability of part of the funds of a user, the DoS rules might apply.

i. The issue causes locking of funds for users for more than a week.
ii. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.
Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

Point 1) For this to cause unavailability of part of the funds due to temporal penalties for more than a week, big donations must be made which makes the attack unfeasible. Point 2) Claiming rewards is not time sensitive.

For all of the above, I believe that this issue doesn't warrant medium severity.

[sherlock-admin3]

Escalate.

This issue should be Low/Informational.

In order to "exploit" this vulnerability, the attacker has to donate a sufficient amount to the victim to move the avgStart parameter enough. The inability of the victim to withdraw will be temporal and not permanent because as time passes, the penalty decreases, so the issue is incorrect when states that will lead to stuck funds.

Additionally, as this issue is about causing temporal unavailability of part of the funds of a user, the DoS rules might apply.

i. The issue causes locking of funds for users for more than a week.
ii. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.
Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

Point 1) For this to cause unavailability of part of the funds due to temporal penalties for more than a week, big donations must be made which makes the attack unfeasible. Point 2) Claiming rewards is not time sensitive.

For all of the above, I believe that this issue doesn't warrant medium severity.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xsimao]

The issue is valid because:

1. It doesn't need to be an attacker, the same user can use one wallet to deposit into another, triggering this bug
2. It always leads to stuck funds for longer and directly to loss of funds in some cases

   Additionally, in case they decide to withdraw after depositing, before the rewards vest again, they will lose these funds to savings.

3. Due to 1. Making the funds stuck for a lot of time is possible and has no significant requirements (up to 24 weeks as stated in the issue)

[santiellena]

1) Agree. However, I still consider it borderline Medium/Low 2) Temporary unavailability is not loss of funds. In which cases loss of funds occurs? 3) As stated in the issue, there actually are significant requirements. The user has to duplicate its staking position in that deposit from other wallet.

I will keep the Escalation so the HoJ reviews it. I think both are fair points.

[0xsimao]

Loss of funds occurs when the user withdraws after depositing. Instead of claiming the rewards on the deposit, avgStart would increase without claiming. Then, when withdrawing, here, on claimWithdraw(), it transfers these rewards to savings, when they should have gone to the user. So the rewards would be lost for the user on unstake. It's not like he can unstake and claim the rewards later, a part of them or all would be lost.

[santiellena]

You are right @0xsimao, in case of staking and then unstaking that will lead to loss of accrued rewards. Unluckily, I didn't have time to check your point before to delete the escalation.

[WangSecurity]

I agree with @0xsimao arguments and believe medium severity is appropriate here. If you need a deeper analysis from my side, let me know, but I don't see a point in repeating the same points. Planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• santiellena: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/exactly/protocol/pull/749

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.