Munchables - 0xleadwizard's results

Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/main/src/managers/LockManager.sol#L256-L268

Vulnerability details

Impact

Users can change the lock duration to a shorter duration, which is unintended.

Proof of Concept

Before changing the lock duration, it is checked that duration + block.timestamp >= unlockTime, This is so that the user can't set a duration that unlocks before the previous lock.

// check they are not setting lock time before current unlocktime
 if (
     uint32(block.timestamp) + uint32(_duration) <
     lockedTokens[msg.sender][tokenContract].unlockTime
 ) {
     revert LockDurationReducedError();
 }

But while updating the lock duration, lastLockTime instead of block.timestamp is used which makes the previous check ineffective.

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

Example:

1. User creates a lock for a token, where unlockTime = 1000 and block.timestamp = 0 = lastLockTime
2. Now block.timestamp is 500, User calls setLockDuration with duration = 501, the check will pass since (500 + 501 >= 1000), then the unlockTime will be set to 0 + 501 = 501
3. So user will be able to unlock in the next block

Recommended Mitigation Steps

Use the below snippet here

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

There is inconsistency in validation between approving & disapproving the proposed price.

Proof of Concept

While disapproving a proposal, whether the same user has approved the same proposal or not is checked

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

But when approving the proposal, no check is there for whether the same user has disapproved it or not.

if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId)
    revert ProposalAlreadyApprovedError();

Recommended Mitigation Steps

Perform the below check in approveUSDPrice

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

Assessed type

Invalid Validation