Munchables - Tychai0s's results

Lines of code

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

Vulnerability details

Impact

Function setLockDuration(uint256 _duration) uses block.timestamp instead of lockedTokens[msg.sender][tokenContract].lastLockTime to validate the new duration doesn't make the lock to be unlocked earlier than originally set. This makes possible for a user to reduce their lockTime even when they have locked tokens.

Proof of Concept

In LockManager.sol:256, the contract uses block.timestamp instead of lastLockTime to check if the new lock duration extends the current unlock time. This allows users to reduce their lock time even when they have locked tokens, which is likely unintended behavior.

// Setup in gist https://gist.github.com/Tychaios/e35d1716f8dc7739554fb19ab0ac60a1
function testUserCanReduceUnlockTimeBySettingNewDuration() external {
        address alice = vm.addr(0x1);
        fundEther(alice, 1000 ether);

        // Lock token so it shouldn't be possible to reduce lock duration
        vm.startPrank(alice);
        lockManager.lock{value: 1 ether}(address(0x0), 1 ether);
        uint256 balanceBeforeUnlocking = address(alice).balance;
        // Retrieve the locked token details for Alice for the token at address(0x0)
        (,,, uint256 unlockTimeBefore) = lockManager.lockedTokens(alice, address(0x0));

        // notice newDuration is half the duration of the lock
        uint256 newDuration = 10 days;

        vm.warp(block.timestamp + 11 days);
        lockManager.setLockDuration(newDuration);

        (,,, uint256 unlockTimeAfter) = lockManager.lockedTokens(alice, address(0x0));

        lockManager.unlock(address(0), 1 ether);

        uint256 balanceAfterUnlocking = address(alice).balance;
        assertGt(balanceAfterUnlocking, balanceBeforeUnlocking);
        assertLt(unlockTimeAfter, unlockTimeBefore);
        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of using block.timestamp, the contract should use lastLockTime when checking if the new lock duration extends the current unlock time. This will prevent users from reducing their lock time while tokens are locked.

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The contract allows a PriceFeed to both approve and disapprove the same USD price proposal, leading to an inconsistent and invalid state where a single entity has conflicting actions on the same proposal.

Proof of Concept

In LockManager.sol:177, the approveUSDPrice function does not check if the msg.sender has previously disapproved the proposal before allowing them to approve it. The contract already has a disapprovals mapping to track disapprovals, but this mapping is not being checked in the approveUSDPrice function. This means that a PriceFeed can potentially both approve and disapprove the same proposal, leading to an inconsistent state.

Tools Used

Manual Review

Recommended Mitigation Steps

To prevent this inconsistency, the approveUSDPrice function should check the disapprovals mapping to ensure that the msg.sender has not previously disapproved the proposal. If they have, the function should revert to maintain consistency.

function approveUSDPrice(uint256 _price) external onlyOneOfRoles([Role.PriceFeed_1, Role.PriceFeed_2, Role.PriceFeed_3, Role.PriceFeed_4, Role.PriceFeed_5]) {
    if (usdUpdateProposal.proposer == address(0)) revert NoProposalError();
    if (usdUpdateProposal.proposer == msg.sender) revert ProposerCannotApproveError();
    if (usdUpdateProposal.approvals[msg.sender] == _usdProposalId) revert ProposalAlreadyApprovedError();
    if (disapprovals[msg.sender] == _usdProposalId) revert ProposalAlreadyDisapprovedError();
    if (usdUpdateProposal.proposedPrice != _price) revert ProposalPriceNotMatchedError();

    usdUpdateProposal.approvals[msg.sender] = _usdProposalId;
    emit ProposalApproved(msg.sender, _price);
// ...
}

By checking the disapprovals mapping, the function will ensure that a PriceFeed cannot both approve and disapprove the same proposal, maintaining consistency in the contract's state.

Issue Type

Governance

Assessed type

Governance