novaman33 - Wrong amount is used when calculation avgDuration in MlumStaking.sol

[sherlock-admin4]

novaman33

High

Wrong amount is used when calculation avgDuration in MlumStaking.sol

Summary

In the MlumStaking the wrong amount is used when calculating the avgDuration for the lock in addToPosition function. The right amount would be the amountToAdd after the _transferSupportingFeeOnTransfer function is called.

Vulnerability Detail

When a fee-on-transfer token is used there is a fee when the users send tokens to the contract. The actual amount that the user has sent is being calculated in _transferSupportingFeeOnTransfer. However when calculating the new lock duration the amount before the fees is taken which means that the duration will be longer than it should be.

Impact

Everytime when a fee-on-transfer token is used, the duration calculated will be longer it should be. This leads to systematical error causing lock times to be longer than they should be.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/7fd1a65b76d50f1bf2555c699ef06cde2b646674/magicsea-staking/src/MlumStaking.sol#L410

Tool used

Manual Review

Recommendation

Use the amountToAdd after _transferSupportingFeeOnTransfer is called.

Duplicate of #545

[novaman33]

Escalate,
This type of issue should not be grouped with the other erc20 issues because the root cause is different. The support of this erc20 is clear by the developers. It is the use of the wrong variable that is the problem not the weird erc20 itself.

[sherlock-admin3]

Escalate,
This type of issue should not be grouped with the other erc20 issues because the root cause is different. The support of this erc20 is clear by the developers. It is the use of the wrong variable that is the problem not the weird erc20 itself.

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.

[0xSmartContract]

This issue occurs with tokens with a fee-on-transfer. Such tokens charge a fee during the transfer and the amount sent may differ from the amount received. The avgDuration calculation is calculated incorrectly because it does not take into account the correct amount after this deduction.

Grouping the category in this way allows auditors and sponsors to better understand the general problems caused by non-standard ERC20 tokens. This way, the report is presented within a consistent theme.

This perspective is resolutely maintained during the entire project audit, if we were to evaluate it in this way, issue number 545 would need to be divided into 33 separate parts.

#545 (comment)

As a result, Duplicate is correct clearly

[WangSecurity]

@novaman33 I see you also escalated #545 itself to change the duplications. Hence, I believe it would be fair to reject this escalation and change the duplication if needed under #545 escalation, so the lead judge doesn't suffer several penalties for almost the same escalations.

Hope for your understanding, planning to reject this escalation.

[WangSecurity]

Result:
Medium
Duplicate of #545

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• novaman33: rejected