zarkk01 - ```_requireOnlyOperatorOrOwnerOf``` does not correctly check the owner or the operator of the position leading to anyone can adjust the duration of a ```LockingPosition``` by adding to it. #665
_requireOnlyOperatorOrOwnerOf does not correctly check the owner or the operator of the position leading to anyone can adjust the duration of a LockingPosition by adding to it.
Vulnerability Detail
The requireOnlyOperatorOrOwnerOf function is supposed to check if the caller of addPosition in MlumStaking contract is, actually, the owner of the LockingPosition or authorized. However, in the way that the function call of _isAuthorized call is implemented the requireOnlyOperatorOrOwnerOf will always return true. We can see the the _isAuthorized function of ERC721 here :
/**
* @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);
}
In MlumStaking, msg.sender is passed in both owner and spender params without checking if the msg.sender is the owner of the NFT as stated in the comments of the _isAuthorized function of ERC721. This results to anyone can call addPosition function for whichever NFT they want to. By adding to the position, an attacker can adjust the duration of any NFT position and can prevent the actual owner of the NFT to withdraw their funds or vote in Voter contract.
Impact
Anyone can change the duration of a LockingPosition can lead to a DoS attack on the actual owner of the position since the lockDuration of the position was selected by the owner so to serve his needs. By extending or reducing the duration of the position, the attacker can prevent the owner from withdrawing his funds or voting in the Voter contract among other problems for the actual owner which does not equal the extra amount in the position that the attacker added.
Proof of concept
This PoC demonstrates the scenario where an attacker DoS the withdrawal of the actual owner of the LockingPosition by adding to it a very tiny amount and extending the duration of it.
To understand better this vulnerability, add this test in MlumStakingTest.sol and run forge test --mt testWithdrawDOSbyAddingToPosition:
function testWithdrawDOSbyAddingToPosition() public {
_stakingToken.mint(ALICE, 100 ether);
_stakingToken.mint(BOB, 1 ether);
vm.startPrank(ALICE);
_stakingToken.approve(address(_pool), 50 ether);
_pool.createPosition(1 ether, 2 days);
vm.stopPrank();
skip(1 days);
vm.expectRevert();
vm.prank(ALICE);
_pool.withdrawFromPosition(1, 0.5 ether);
skip(1 days);
// as long as the malicious Bob deposits amountAdd > amountStaked / (secondsInitDuration - 1) the withdraw of Alice will fail because the duration gonna be extended.
vm.startPrank(BOB);
_stakingToken.approve(address(_pool), 5787070527029);
_pool.addToPosition(1, 5787070527029);
vm.stopPrank();
vm.expectRevert();
vm.prank(ALICE);
_pool.withdrawFromPosition(1, 1 ether);
}
Consider making this change in the _requireOnlyOperatorOrOwnerOf function so to implement correctly the check :
function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {
// isApprovedOrOwner: caller has no rights on token
- require(ERC721Upgradeable._isAuthorized(msg.sender, msg.sender, tokenId), "FORBIDDEN");
+ require(ERC721Upgradeable._isAuthorized(ERC721Upgradeable.ownerOf(tokenId), msg.sender, tokenId), "FORBIDDEN");
}
zarkk01
High
_requireOnlyOperatorOrOwnerOf
does not correctly check the owner or the operator of the position leading to anyone can adjust the duration of aLockingPosition
by adding to it.Vulnerability Detail
The
requireOnlyOperatorOrOwnerOf
function is supposed to check if the caller ofaddPosition
inMlumStaking
contract is, actually, the owner of theLockingPosition
or authorized. However, in the way that the function call of_isAuthorized
call is implemented therequireOnlyOperatorOrOwnerOf
will always return true. We can see the the_isAuthorized
function ofERC721
here :In
MlumStaking
,msg.sender
is passed in bothowner
andspender
params without checking if themsg.sender
is the owner of the NFT as stated in the comments of the_isAuthorized
function ofERC721
. This results to anyone can calladdPosition
function for whichever NFT they want to. By adding to the position, an attacker can adjust the duration of any NFT position and can prevent the actual owner of the NFT to withdraw their funds or vote inVoter
contract.Impact
Anyone can change the duration of a
LockingPosition
can lead to a DoS attack on the actual owner of the position since thelockDuration
of the position was selected by the owner so to serve his needs. By extending or reducing the duration of the position, the attacker can prevent the owner from withdrawing his funds or voting in theVoter
contract among other problems for the actual owner which does not equal the extra amount in the position that the attacker added.Proof of concept
This PoC demonstrates the scenario where an attacker DoS the withdrawal of the actual owner of the
LockingPosition
by adding to it a very tiny amount and extending the duration of it. To understand better this vulnerability, add this test inMlumStakingTest.sol
and runforge test --mt testWithdrawDOSbyAddingToPosition
:Code Snippet
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L140
Tool used
Manual Review
Recommendation
Consider making this change in the
_requireOnlyOperatorOrOwnerOf
function so to implement correctly the check :Duplicate of #378