Upon withdrawal of deposit, the claimer will be called with onDepositBurned.
This happens after the claimer shares have been updated, but before the underlying has been sent away from the contract.
Therefore the claimer can reenter the contract, at an intermediary state where the invariants are not held.
Impact
Claimer can claim more yield than he deserves.
Proof of Concept
The withdraw function iterates on all deposits, and then sends underlying only at the end.
For each deposit (while iterating), Sandclock first updates the shares state, and then callsonDepositBurned.
So the claimer can reenter the vault after the shares have been updated but before underlying was sent.
POC for exploit:
Alice deposits 100 tokens, with claims that go 50% to bob and 50% to exploiter
Vault generates 10 yield
Alice withdraws the deposit which has exploiter as claimed
The exploiter, on onDepositBurned, claims his yield from the contract. Since the underlying has not been sent yet, the state is not consistent. Exploiter gets 9.16 yield instead of 5.
Recommended Mitigation Steps
Usual reentrancy protection: conform with checks-effects-interaction pattern, or add reentrancy lock.
r2moon
commented
2 years ago
Handle
kenzo
Vulnerability details
Upon withdrawal of deposit, the claimer will be called with
onDepositBurned. This happens after the claimer shares have been updated, but before the underlying has been sent away from the contract. Therefore the claimer can reenter the contract, at an intermediary state where the invariants are not held.Impact
Claimer can claim more yield than he deserves.
Proof of Concept
The
withdrawfunction iterates on all deposits, and then sends underlying only at the end. For each deposit (while iterating), Sandclock first updates the shares state, and then callsonDepositBurned. So the claimer can reenter the vault after the shares have been updated but before underlying was sent.POC for exploit:
onDepositBurned, claims his yield from the contract. Since the underlying has not been sent yet, the state is not consistent. Exploiter gets 9.16 yield instead of 5.Recommended Mitigation Steps
Usual reentrancy protection: conform with checks-effects-interaction pattern, or add reentrancy lock.