Open sherlock-admin4 opened 1 month ago
Users Can Add Amounts Instead of Each Other:
We see that the _requireOnlyOperatorOrOwnerOf
function always returns true using the msg.sender and msg.sender arguments, and therefore any user can add amounts to other users' staking positions.
This can lead to harm to the positions of others and lead to involuntary changes.
To solve this problem, the _requireOnlyOperatorOrOwnerOf
function must be updated and it must be properly checked whether the caller is authorized or not.
PR: https://github.com/metropolis-exchange/magicsea-staking/pull/9
We fixed this issue already for an other audit. Basically we removed all isApprover... / requireOnlyOperator... checks and check for ownership instead. The linked PR just highlight the _checkOwnerOf function.
The protocol team fixed this issue in the following PRs/commits: https://github.com/metropolis-exchange/magicsea-staking/pull/9
The Lead Senior Watson signed off on the fix.
Yashar
Medium
Attacker can manipulate the
lockDuration
of other users positionsSummary
The
_requireOnlyOperatorOrOwnerOf
check inaddToPosition
can be bypassed, allowing attackers to manipulate thelockDuration
of other users positions, impacting voting eligibility and preventing users from participating in voting and earning rewards.Vulnerability Detail
The
_requireOnlyOperatorOrOwnerOf
check inaddToPosition
ensures that the caller is either the owner or the operator of the tokenId:The
_requireOnlyOperatorOrOwnerOf
function performs this verification by callingERC721Upgradeable._isAuthorized
withmsg.sender
as both theowner
andspender
:Since
_isAuthorized
returnstrue
whenowner == spender
, this check always evaluates totrue
regardless of the caller's identity:Any function implementing this check is vulnerable to bypass, but currently only
addToPosition
is affected. An attack scenario involves an attacker manipulating thelockDuration
of another user's position by donating a small amount (1 wei). When users create a position,lockDuration
dictates how long their asset is locked. This duration impacts rewards, voting eligibility, etc. If an attacker increases another user's position viaaddToPosition
, the function recalculateslockDuration
based on newamountToAdd
and existing lock times:A malicious user can call
addToPosition
and donate 1 wei to a position, thereby manipulating the lockDuration of that position, which will have multiple consequences for the victim.amount
and 2 weeks aslockDuration
.lockDuration
is equal to thePeriodDuration
, which is 2 weeks, she is eligible to participate in voting.addToPosition
and donate 1 wei to Alice's positionlockDuration
is recalculated, potentially reducing it to 1 week.vote
but her transaction reverts due toInsufficientLockTime
error.Coded PoC
Above scenario is implemented in this test Please make a file named
Ninja.t.sol
in this path:/test/
and paste the following test code in it:Run the test:
Output:
Impact
This bug has multiple impacts on rewards calculation, voting eligibility, etc., as highlighted in this report, particularly preventing users from participating in voting, thereby denying them access to rewards.
Code Snippet
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L398 https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L142 https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L409
Tool used
Manual Review
Recommendation