Users can unlock after their lock period ends. The unlock time is denoted by the unlockTime field of the struct. This check is done in the unlock function.
if (lockedToken.unlockTime > uint32(block.timestamp)) {
revert TokenStillLockedError();
}
However, when a user creates a lock over an existing lock, the time gets reset. The previous unlock time is overwritten with the new unlock time. This is done in the lock function.
So if a fresh lock is opened, the unlockTime is refreshed and pushed back by the duration amount. The issue is that any user can trigger this via the lockOnBehalf function. This function allows users to lock tokens on behalf of other users. This will also reset the lock time.
So if a user waits untl the unlock time to unlock their stake, another user can come in and reset the lock time by depositing a single wei through the lockOnBehalf function. The original user will not be able to unlock their stake until the new lock time is reached.
Since this prevents unlocks, this can be considered a high severity issue.
Proof of Concept
Say Alice has a lock expiring today. Her lockDuration is set to 10 days.
Bob calls the lockOnBehalf function with Alice's address and locks some tokens. Alice's unlockTime is now pushed back by 10 days.
Alice is unable to unlock her tokens. This can be done indefinitely to prevent Alice from unlocking her tokens.
Tools Used
Manual Review
Recommended Mitigation Steps
Consider removing the lockOnBehalf function. Or, add a check so that only a set of addresses approved by the user can use lockOnBehalf for the target user.
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L410-L411
Vulnerability details
Impact
Users can unlock after their lock period ends. The unlock time is denoted by the
unlockTime
field of the struct. This check is done in theunlock
function.However, when a user creates a lock over an existing lock, the time gets reset. The previous unlock time is overwritten with the new unlock time. This is done in the
lock
function.So if a fresh lock is opened, the
unlockTime
is refreshed and pushed back by the duration amount. The issue is that any user can trigger this via thelockOnBehalf
function. This function allows users to lock tokens on behalf of other users. This will also reset the lock time.So if a user waits untl the unlock time to unlock their stake, another user can come in and reset the lock time by depositing a single wei through the
lockOnBehalf
function. The original user will not be able to unlock their stake until the new lock time is reached.Since this prevents unlocks, this can be considered a high severity issue.
Proof of Concept
Say Alice has a lock expiring today. Her
lockDuration
is set to 10 days.Bob calls the
lockOnBehalf
function with Alice's address and locks some tokens. Alice'sunlockTime
is now pushed back by 10 days.Alice is unable to unlock her tokens. This can be done indefinitely to prevent Alice from unlocking her tokens.
Tools Used
Manual Review
Recommended Mitigation Steps
Consider removing the
lockOnBehalf
function. Or, add a check so that only a set of addresses approved by the user can uselockOnBehalf
for the target user.Assessed type
Access Control