neogranicen - A users lock time can be extended indefinitely by an attacker

[sherlock-admin4]

neogranicen

Medium

A users lock time can be extended indefinitely by an attacker

Summary

Anyone can add to any position, and by doing so, extend the lock time indefinitely, making the original owner unable to withdraw except in an emergency due to a misuse of _isAuthorized.

Vulnerability Detail

Its supposed that addToPosition can only be called by lsNFT's owner or operators and to do this check _requireOnlyOperatorOrOwnerOf is used which uses _isAuthorized as is shown below

function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {  
// isApprovedOrOwner: caller has no rights on token  
require(ERC721Upgradeable._isAuthorized(msg.sender, msg.sender, tokenId), "FORBIDDEN");  
}  

We can see that the msg.sender is passed as both the spender and owner to this function, and if we look at _isAuthorized,we see the following:

/**  
* @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in  
* particular (ignoring whether it is owned by `owner`).  
*  
* WARNING: This function assumes that `owner` is the actual owner of `tokenId` and does not verify this  
* assumption.  
*/  
function _isAuthorized(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) {  
return spender != address(0) && (owner == spender || isApprovedForAll(owner, spender) || _getApproved(tokenId) == spender);  
}  

As is clear from the warning, the function does not check whether the owner passed to it is the real owner, and if the owner is equal to the spender, the check succeeds immediately. Since _requireOnlyOperatorOrOwnerOf passed msg.sender as both owner and spender any address will be able to pass this check and deposit to any token ID, even if the ID has not been minted yet.

The lock duration is updated upon each successful addToPosition call by the following formula:

uint256 avgDuration = (remainingLockTime * position.amount + amountToAdd * position.initialLockDuration) / (position.amount + amountToAdd);  

This makes it possible for an attacker to keep depositing to a certain position and extending the locktime.

Impact

Because of the above issue, anyone can increase the user to an indefinite amount of time ranging from hours to years and by doing so exposing the user to price fluctuations he did not plan for.

The amout of time the position will be extended by depends on

• the amount the attacker added
• the initial locktime the user sat
• the remaining time

The following graph shows that if a user has $X$ locked tokens and $I$ intial lock time, by adding $0.19672X$ to his position, you will extend it by $0.16438356164$ of $I$ (without accounting for small precesion loss from the solidity fixed point numbers model).

In the graph below, we assume the remaining time to be zero, but as the remaining time increases, the duration added by locking decreases, but this decrease will be trivial if the remaining time is short, like a day or less and this will be demonstrated in the poc.

But the attacker can always wait when it's near for the lock to expire and add more lock time without exposing himself to the effect of the remaining time.

If we assume the user deposited 1000 tokens and locked them for a year and 364 days passed, then the attacker can, by depositing 19.672% of the user's locked amount, lock the user's funds for 60 additional days (minus a small amount due to precision loss and the effect of remaining time)

If the attacker after nearly 60 days wants to lock the position for another 60 days, he will need to pay 19.672% of $( X + 0.19672X )$ and so on.

The following is poc that demostrates the previous attack senario

Add the following to MlumStaking.t.sol

function testAttackPoc() public {  
vm.warp(0);

address attacker = makeAddr("attacker");  
_rewardToken.mint(address(_pool), 100_000_000);  
_stakingToken.mint(ALICE, 1000 ether);  
_stakingToken.mint(attacker, 196.72 ether);

vm.startPrank(ALICE);  
_stakingToken.approve(address(_pool), 1000 ether);  
_pool.createPosition(1000 ether, 365 days);  
vm.stopPrank();

skip(364 days);  
uint Alice_LockTime_Before = 1 days;// 364 of the 365 days passed the remaining lock time is 1 day

vm.startPrank(attacker);  
_stakingToken.approve(address(_pool), 196.72 ether);  
_pool.addToPosition(1, 196.72 ether);  
vm.stopPrank();

uint Alice_LockTime_After = _pool.getStakingPosition(1).lockDuration;

assertEq(Alice_LockTime_After, Alice_LockTime_Before + 60 days - 14232); // 14232 seconds/4 hours less than 60 days because of remainig time caused deviation
}  

Code Snippet

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

Tool used

Desmos
Manual Review
Foundry

Recommendation

Instead of using _isAuthorized you can do the same thing it does safely by just checking whether the caller is the owner of the NFT or whether he is approved of the NFT.

Duplicate of #378