howlbot-integration[bot] commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L257 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L265-L267

Vulnerability details

Summary

In the current implementation of LockManager::setLockDuration, user can manipulate the unlock time, resulting into decreasing of the unlockTime.

Vulnerability Detail

In the current implementation of LockManager::setLockDuration, users have the ability to decrease the lock time of their funds. This arises from the function's conditional check utilizing block.timestamp, which inherently increases over time. Consequently, as users attempt to reduce the lock duration, the block.timestamp continues to increment, leading to a decrease in the unlock time.

if (uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) {
   revert LockDurationReducedError();
}

uint32 lastLockTime = lockedTokens[msg.sender][tokenContract].lastLockTime;
lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration);

Impact

Allowing users to reduce unlock time can have multifaceted impacts on the platform. Firstly, it facilitates early access to locked resources, potentially disrupting planned release schedules and causing unforeseen consequences.

Moreover, there's a potential increase in NFT acquisition as users exploit the system to acquire more NFTs than intended, which can affect asset scarcity and value. Additionally, premature unlocking could disrupt economic models tied to resource release schedules, impacting supply-demand dynamics, pricing mechanisms, and overall platform stability.

Code Snippet

if (uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime) {
   revert LockDurationReducedError();
}

uint32 lastLockTime = lockedTokens[msg.sender][tokenContract].lastLockTime;
lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration);

Proof Of Concept

Add below test to setLockDurationTests.test.ts and run tests with yarn build && yarn test.
```
...
import { testClient, ONE_DAY, ONE_WEEK } from "../../utils/consts";
...
```

... initialLockDuration = maxLockDuration - 50n; ...

it("should revert with LockDurationReducedError when reducing lock duration", async () => { const setLockDurationTxHash = await testContracts.lockManager.contract.write.setLockDuration([initialLockDuration + 3n], { account: alice, }); const txReceipt = await assertTxSuccess({ txHash: setLockDurationTxHash }); const block = await testClient.getBlock({ blockHash: txReceipt.blockHash });

const lockedTokens = await testContracts.lockManager.contract.read.getLocked([alice]); assert(lockedTokens instanceof Array); const lockedEthToken = lockedTokens.find((t) => t.tokenContract === zeroAddress); assert.ok(lockedEthToken);

assert(lockedEthToken.lockedToken.lastLockTime + Number(initialLockDuration) + Number(2n) < lockedEthToken.lockedToken.unlockTime);

await testClient.setNextBlockTimestamp({ timestamp: block.timestamp + ONE_DAY, }); await assert.rejects( testContracts.lockManager.contract.simulate.setLockDuration([initialLockDuration + 2n], { account: alice, }), (err: Error) => assertContractFunctionRevertedError(err, "LockDurationReducedError") );

assert(lockedEthToken.lockedToken.lastLockTime + Number(initialLockDuration) + Number(1n) < lockedEthToken.lockedToken.unlockTime);

await testClient.setNextBlockTimestamp({ timestamp: block.timestamp + (ONE_DAY + ONE_WEEK), }); await assert.rejects( testContracts.lockManager.contract.simulate.setLockDuration([initialLockDuration + 1n], { account: alice, }), (err: Error) => assertContractFunctionRevertedError(err, "LockDurationReducedError") ); });


## Tool used
Manual Review

## Recomendation
Consider using `lockedTokens[msg.sender][tokenContract].lastLockTime` instead of `uint32(block.timestamp)` in the `LockManager::setLockDuration` function. 

It will look like this:
```solidity
function setLockDuration(uint256 _duration) external notPaused {
    if (_duration > configStorage.getUint(StorageKey.MaxLockDuration))
        revert MaximumLockDurationError();

    playerSettings[msg.sender].lockDuration = uint32(_duration);
    // update any existing lock
    uint256 configuredTokensLength = configuredTokenContracts.length;
    for (uint256 i; i < configuredTokensLength; i++) {
        address tokenContract = configuredTokenContracts[i];
        if (lockedTokens[msg.sender][tokenContract].quantity > 0) {
        // check they are not setting lock time before current unlocktime
        if (
            lockedTokens[msg.sender][tokenContract].lastLockTime +
            uint32(_duration) <
            lockedTokens[msg.sender][tokenContract].unlockTime
        ) {
            revert LockDurationReducedError();
        } 

        uint32 lastLockTime = lockedTokens[msg.sender][tokenContract].lastLockTime;
        lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration);
        }
    }

    emit LockDuration(msg.sender, _duration);
}

Assessed type

Invalid Validation

c4-judge commented 3 months ago

alex-ppg marked the issue as duplicate of #89