Munchables - EPSec's results

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

1. 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:

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

Judge has assessed an item in Issue #548 as 2 risk. The relevant finding follows:

Low 02 User Can Approve And Dissaprove

Summary

There are two functions responsible for approving and disapproving proposals. Users are expected to either approve or disapprove a proposal, but not both. Currently approve function doesn't check if the user was already disapproved.

Vulnerability Detail

In the LockManager contract, there are two functions for managing proposals: approveUSDPrice and disapproveUSDPrice. Users are expected to either approve or disapprove a proposal, but not both.

Disapprove Function:

The disapproveUSDPrice function correctly checks if the user has already either approved or disapproved the proposal. If either condition is true, the function reverts to prevent the user from performing conflicting actions.

Approve Function:

The current implementation of the approveUSDPrice function only checks if the user has already approved the proposal. It does not verify if the user has previously disapproved the proposal, creating an inconsistency.

This omission allows a user who has disapproved a proposal to later approve it, which contradicts the intended logic and can lead to inconsistent proposal states.

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

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

Impact

The lack of a disapproval check in the approveUSDPrice function can result in a proposal being both approved and disapproved by the same user, leading to inconsistencies.

Code Snippet

None

Tool used

Manual Review

Recommendation

Add this line of code tot LockManager:approveUSDPrice function:

if (usdUpdateProposal.disapprovals[msg.sender] == _usdProposalId) 
    revert ProposalAlreadyDisapprovedError();