Handle

harleythedog

Vulnerability details

Impact

Consider the stake padding example given in the contest description here: <https://code4rena.com/contests/2021-11-malt-finance-contest&gt;. At the end of the example, User A has 100 bonded LP and has 100 stake padding. User B has 100 bonded LP and 200 stake padding. There are 300 in rewards in the system. For this example, suppose that all of this reward has been vested, so that the balance of the contract is 300.

Now, lets continue the example and follow along with the code in AbstractRewardMine.sol. Suppose user A unbonds 50% of their position. In the onUnbond function, the following order of events will happen:

• User A is transferred their 200 reward tokens. _userWithdrawn[userA] is set to 200, and _globalWithdrawn is set to 200. The balance of the contract is now 100 reward tokens.
• _reconcileWithdrawn is called and results in _userWithdrawn[userA] and globalWithdrawn being decreased by half. So, _userWithdrawn[userA] is set to 100, and _globalWithdrawn is set to 100 after this call.
• Half of User A’s stake padding is removed, so User A now only has 50 stake padding. Also, we can assume that whatever function called onUnbond has appropriately removed 50 LP tokens from User A’s balance.

Now, suppose that User A calls withdrawAll (a public function allowing remaining reward to be withdrawn). Since User A has already received their 200 reward tokens, we expect earned(User A) should return 0 in this function, but lets look closer into earned. We know that totalDeclaredReward + totalStakePadding = 100 + (50 + 200) = 350 (since we have 100 reward tokens in the contract, User A has 50 stake padding, User B has 200 stake padding). This means that _getFullyPaddedReward(User A) returns 350*(50/150) = 116.67 (since User A has 50 LP bonded relative to 150 total LP bonded). This means that balanceOfRewards(User A) will return 116.67 - 50 = 66.67 (since User A has 50 stake padding). There is a total of 100 reward tokens in the contract, so totalDeclaredReward() returns 100. This means that earned(User A) will return (totalReleasedReward() * 66.67 / 100) - _userWithdrawn[userA] = ((100 + 50) * 66.67 / 100) - 50 = 50. So, User A will be transferred another 50 reward tokens after calling withdrawAll. These 50 tokens belong to User B and not User A, so user A is effectively stealing User B’s rewards and making a profit. This could even happen unintentionally.

Proof of Concept

This problem persists in all different implementations of onUnbond:

<https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/AbstractRewardMine.sol#L39&gt;

<https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/AuctionPool.sol#L55&gt;

<https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/ERC20VestedMine.sol#L36&gt;

Tools Used

Inspection

Recommended Mitigation Steps

I am not really sure which specific part of the logic is going wrong here. I believe the underlying issue is that the full reward owed is always being paid out to the user in onUnbond, but _userWithdrawn is decrementing based on what share of the user’s LP is being unbonded. These calculations are hard to follow even if they were correct, so I would recommend looking into an easier way to track how much reward token to send to a user. For example, how about never decrementing _userWithdrawn, and you always keep track of the cumulative sum of rewards that the user has earned? I believe this is a more common approach used in DeFi.

---

The text was updated successfully, but these errors were encountered:

All reactions