The claimedBalance mapping is not updated correctly

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0xbb99072b38a8f2aa950249aacbec10bbb8eb7776569381f5e13ba0be71b056d1 Severity: high

Description: Description\ The claimRewards function is intended to facilitate users in claiming rewards. The primary issue in this function is the order and how the claimedBalance mapping is updated relative to the subtraction of accumulatedBalance.

Here is the relevant part of the code:

uint256 claimableBalance = accumulatedBalance - claimedBalance[withdrawalAddress];
// Update claimed balance mapping
claimedBalance[withdrawalAddress] = accumulatedBalance;

This may result in users being able to claim rewards without any prior record, making the mapping less useful for its intended purpose.

With the code above, it discards any prior records of claimed balances, and for every claim, the claimed balance is updated to match the current accumulatedBalance.

Attack Scenario\ The claimedBalance mapping is not updated correctly.

Attachments https://github.com/dappnode/mev-sp-contracts/blob/3929e24ea288d697d38948b8690c8c2028e5042b/contracts/DappnodeSmoothingPool.sol#L298-L337

1. Proof of Concept (PoC) File

uint256 claimableBalance = accumulatedBalance - claimedBalance[withdrawalAddress];
// Update claimed balance mapping
claimedBalance[withdrawalAddress] = accumulatedBalance;

2. Revised Code File (Optional)

I suggest a check like so:

// In the claimRewards function
if (claimedBalance[withdrawalAddress] == 0) {
    claimedBalance[withdrawalAddress] = accumulatedBalance;
} else {
    uint256 claimableBalance = accumulatedBalance - claimedBalance[withdrawalAddress];
    claimedBalance[withdrawalAddress] += accumulatedBalance; // Update the claimed balance
    // Rest of the claimRewards logic
}

[invocamanman]

The current logic of the contract it's correct. if you disagree i encourage you to create a test file to prove the bug.

In the PoC provided, you are calculating the claimableBalance as accumulatedBalance - claimedBalance[withdrawalAddress]; And claimedBalance[withdrawalAddress] += accumulatedBalance

Be aware that doing this calculation, for example if a user withdraws:

• 1 ether ( total accumulated balance on the leaf 1 ether)
• 0.1 ether ( total accumulated balance on the leaf 1.1 ether) The claimedBalance should match the total accumulated balance on the leaf which is 1.1 ether. BUt using your proposar will end up suming 2.1 ether

[ololade97]

@invocamanman, my point lies in this line of code:

claimedBalance[withdrawalAddress] = accumulatedBalance;

claimedBalance[withdrawalAddress] is meant to serve as a store for accumulatedBalance. However based on the line of code above, it won't store any previous accumulated balance.

[ololade97]

Like your example here:

Be aware that doing this calculation, for example if a user withdraws:

1 ether ( total accumulated balance on the leaf 1 ether) 0.1 ether ( total accumulated balance on the leaf 1.1 ether) The claimedBalance should match the total accumulated balance on the leaf which is 1.1 ether.

claimedBalance will only match the last input of accumulated balance which is 0.1 ether because of this line: claimedBalance[withdrawalAddress] = accumulatedBalance;

The previous 1 ether will be erased from claimedBalance mapping and 0.1 ether will be stored instead.

[invocamanman]

this behavior is intended and it's correct. The accumulate balance, is tracked in the oracle and it will reflect the total balance that this account has earned from the protocol.

Therefore the claim mapping, every time a user wants to withdraw his rewards he will withdraw the difference between the accumulatedBalance amount and the already claimedBalance. Therefore just after claiming, the claimed balance will be the accumulate balance^^

[ololade97]

True that the accumulated balance is tracked on the leaf.

However, the accumulated balance is not removed from the leaf even after it is claimed.

What this means is that a user can reclaim an already claimed accumulated balance more than once.

This is because the mapping that is supposed to track the claimed balance is not updated correctly.

[ololade97]

For example, a user has:

• 1 ether ( total accumulated balance on the leaf 1 ether)
• 0.1 ether ( total accumulated balance on the leaf 1.1 ether). The user first claimed 1 ether. 1 ether will be stored in the claimedBalance mapping.

Then the user claimed 0.1 ether. 0.1 ether will now be stored on the claimedBalance mapping.

A user can claim 1 ether again since it's on the leaf and not removed. The calculation would be:

uint256 claimableBalance = accumulatedBalance - claimedBalance[withdrawalAddress];

This means: uint256 claimableBalance = 1 - 0.1;

And a user claims 0.9 ether again.

[invocamanman]

no, when the user claimed the other 0.1 ether, 1.1 ether will be stored on the leaf

AAAA i got now what u mean. Oki every user ONLY has one address on the claimRoot ( ofc) Note that if that case happen it will revert by math overflow ( in case that for some reason the oracle malfunction) so even then this would not be possible

[ololade97]

There won't be an overflow because the math is within range such as in the example I gave above.

[ololade97]

Also note that overflow will only occur if claimedBalance mapping has tracked and stored all previous accumulatedBalance.

For instance, if a user has:

• 3 ether
• 4 ether

The user claimed both the 3ether and 4 ether accumulatedBalance. And claimedBalance mapping stored all. Meaning that a user has 7 ether in claimedBalance mapping.

If a user wants to reclaim 4 ether for example, there will be an overflow:

uint256 claimableBalance = accumulatedBalance - claimedBalance[withdrawalAddress];

That is: uint256 claimableBalance = 4 - 7;

There will be an overflow. And revert will occur.

So, overflow will occur if claimedBalance mapping has stored all previous accumulatedBalance of a user.

In the code, claimedBalance mapping doesn't store all previous claims. It only stores the last claimed accumulatedBalance.

[invocamanman]

i meant underflow, sorry *

[invocamanman]

it does not work like that: if user claimed both 3 ether and 4 ethers, the accumulated balance is 4. In the first call the user will claim succesfully 3 ethers, and in the second one 1 ehter The claimed balance will be indeed 4ethers ^^

[invocamanman]

The overflow exception could occur only if there's an inconsistency on the oracle, and submit a root, containing an account with an accumulate balance inferior to a previous one. THis should NEVER happen, meant that the oracle is broken, but my point was, that even that case is handled by the smart contract by the underflow exception

[invocamanman]

I think i see very clearly there's no issue here. I suggest that, if you think otherwise, you make a forge ( or whatever tool) script in order to proof the bug. Again just keep on mind that:

• Rewards root will contain one leaf for every withdrawal address, and cannot happen that there's 2 leafs with the same address.
• Oracle will always post an accumulated balance equal or superior to the last one for every account.

[ololade97]

Ok, I will write a foundry test to prove the bug.