Elegant Vanilla Crane - Fee-on-transfer tokens can affect position’s lock duraiton using `MlumStaking::addToPosition()`

[sherlock-admin4]

Elegant Vanilla Crane

Low/Info

Fee-on-transfer tokens can affect position’s lock duraiton using MlumStaking::addToPosition()

Summary

Fee-on-transfer token amount calculations are not taken into account when calculating the position's lockDuration in MlumStaking::addToPosition().

Vulnerability Detail

MlumStaking accepts all kinds of ERC20 tokens, meaning that we can observe fee-on-transfer ones. The protocol takes this into account by introducing _transferSupportingFeeOnTransfer which checks the before and after balance of each token during transfers. However, in the addToPosition() function, the position's lockDuration is calculated, using the amount before any fee-on-transfer changes, but the position's actual amount takes into account fee-on-transfer changes.

 function addToPosition(uint256 tokenId, uint256 amountToAdd) external override nonReentrant {
        _requireOnlyOperatorOrOwnerOf(tokenId);
        require(amountToAdd > 0, "0 amount"); // addToPosition: amount cannot be null

        _updatePool();
        address nftOwner = ERC721Upgradeable.ownerOf(tokenId);
        _harvestPosition(tokenId, nftOwner);

        StakingPosition storage position = _stakingPositions[tokenId];

        // we calculate the avg lock time:
        // lock_duration = (remainin_lock_time * staked_amount + amount_to_add * inital_lock_duration) / (staked_amount + amount_to_add)
        uint256 remainingLockTime = _remainingLockTime(position);
@>   uint256 avgDuration = (remainingLockTime * position.amount + amountToAdd * position.initialLockDuration)
            / (position.amount + amountToAdd); // avgDuration is calculated with amount before fee-on-transfer taxes

        position.startLockTime = _currentBlockTimestamp();
@>  position.lockDuration = avgDuration; // lockDuration is set with amount before fee-on-transfer taxes

        // lock multiplier stays the same
        position.lockMultiplier = getMultiplierByLockDuration(position.initialLockDuration);

        // handle tokens with transfer tax
        amountToAdd = _transferSupportingFeeOnTransfer(stakedToken, msg.sender, amountToAdd);

        // update position
@>  position.amount = position.amount + amountToAdd;  // position is set with amount after fee-on-transfer taxes
        _stakedSupply = _stakedSupply + amountToAdd;
        _updateBoostMultiplierInfoAndRewardDebt(position);

        emit AddToPosition(tokenId, msg.sender, amountToAdd);
    }

Impact

Fee-on-transfer tokens can affect the position’s lock duration, leading to inaccurate calculations.

Code Snippet

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

Tool used

Manual Review

Recommendation

Calculate the position's lockDuration after applying any fee-on-transfer amount changes, the same way it's done in createPosition().

[0xSmartContract]

This issue was submitted as low/info.