CodingNameKiki - A mistake made by depositer will result into losing his funds, which will be stuck in the pool.

[sherlock-admin]

CodingNameKiki

high

A mistake made by depositer will result into losing his funds, which will be stuck in the pool.

Summary

A mistake made by the depositer will result into losing his funds, which will be stuck in the pool.

Vulnerability Detail

Example: Kiki wants to lock his funds and calls the function deposit(). He locks his 5000 tokens for the duration of 2 weeks and provides his address as the "_receiver".

By calling the function deposit(): 1.The tokens will be transferred from Kiki to the pool. 2.The mintAmount will be calculated 3.Kiki's deposit information will be stored into a new Deposit struct in the depositOf mapping. 4.After that the function will mint the corresponding shares for the given amount of tokens to Kiki.

Later Kiki decides that he wants to lock another 1000 of his tokens. Instead of calling the function increaseLock(), he mades the mistake and calls the function deposit() again. This time he provides the amount of 1000 tokens and the same duration of 2 weeks.

The function will push the new deposit information to the already existing Deposit struct from the first deposit Kiki made. However the problem here is that the function will change the amount of 5000 tokens from the first deposit with the new deposit of 1000 tokens and won't actually sum both of the amounts. Same happens for the shares as well, the old "shareAmount" corresponding the amount of 5000 tokens will be replaced with the new shares from the second deposited amount of 1000 tokens.

https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/TimeLockPool.sol#L98-L102

2 weeks later when Kiki calls the function withdraw() to withdraw his tokens. The function will delete Kiki's Deposit struct, will burn the shares and return the tokens. However duo to the fact that userDeposit.shareAmount and userDeposit.amount will be equal to the 1000 tokens from the second deposit Kiki made. The function will successfuly return only the 1000 tokens to Kiki. But his other 5000 tokens from the first deposit he made, will be stuck in the pool.

https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/TimeLockPool.sol#L130

https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/TimeLockPool.sol#L133

Kiki won't be able to withdraw his first deposit of 5000 tokens. Because the function withdraw() checks the deposit information from the Kiki's Deposit struct and both userDeposit.amount and userDeposit.shareAmount will be equal to the second deposit Kiki made for 1000 tokens.

Duo to the fact that the deposit() function minted shares for both of the deposits Kiki made, and only the shares of the second deposit were burned corresponding the 1000 tokens. Kiki won't only lose his first deposit of 5000 tokens, but won't even get the chance to claim the full amount of rewards, he will be able to claim the rewards only for the burned amount of shares corresponding the 1000 tokens.

Simple mistake led to Kiki's losing 5000 tokens out of the 6000 tokens he deposited into the pool from the two deposits. And by following this scenario his 5000 tokens will be stuck in the pool without a way for retrieving them back.

Impact

Duo to the exploit described in "Vulnerability Detail", a simple mistake made by the depositer will lead to losing his funds, which will be stuck in the pool.

Code Snippet

https://github.com/sherlock-audit/2022-10-merit-circle/blob/main/merit-liquidity-mining/contracts/TimeLockPool.sol#L85-L107

Tool used

Manual Review

Recommendation

Consider adding the following changes to avoid this kind of problem from happening: https://gist.github.com/CodingNameKiki/e28ccf6a48df4fcf4f8231a6468e52e1

[federava]

I am not sure if I am getting this right: "The function will push the new deposit information to the already existing Deposit struct from the first deposit Kiki made."

I ellaborate: If I get the example right, when deposit() is called for second time Kiki uses the same _receiver address (his address). When this happens the Deposit struct gets overwritten with the new one, changing (reducing) the amount from 5000 to 1000. I am not able to see how the overwritting part happens as the code pushes (adds) a new Deposit struct in the Deposit array corresponding to Kiki's address, so now the array has both the first and second deposits, an the array length is two. This behaves like this because depositsOf is a mapping that contains an array of Deposit structs per address, mapping(address => Deposit[]), it allows multiple deposits per user.

I haven't been able to see how the struct is overwritten, feel free to provide a code snippet or any additional information.

[jack-the-pug]

This should be invalid. Seems like the auditor didn't realize that depositsOf[_receiver] is an array.