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.
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:
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
}
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.
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
addToPositioncan only be called by lsNFT's owner or operators and to do this check_requireOnlyOperatorOrOwnerOfis used which uses_isAuthorizedas is shown belowWe 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: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
_requireOnlyOperatorOrOwnerOfpassed 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
addToPositioncall by the following formula: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 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.solCode 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
_isAuthorizedyou 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