Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L245-L272 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L311-L398 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L142-L174 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L98-L112 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L401-L427

Vulnerability details

Impact

Detailed description of the impact of this finding.

Vulnerability: Dubious Typecasting in LockManager Functions

Description:

The vulnerability arises from improper typecasting of variables in the setLockDuration, _lock, proposeUSDPrice, configureLockDrop, and unlock functions. Specifically, the conversion from uint256 to uint32 is performed without proper validation, leading to potential truncation and unexpected behavior.

Affected Locations:

setLockDuration Function (src/managers/LockManager.sol lines 245-272):
- playerSettings[msg.sender].lockDuration = uint32(_duration) (line 249)
- uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime (lines 257-258)
- lockedTokens[msg.sender][tokenContract].unlockTime = lastLockTime + uint32(_duration) (lines 265-267)
_lock Function (src/managers/LockManager.sol lines 311-398):
- lockdrop.start <= uint32(block.timestamp) && lockdrop.end >= uint32(block.timestamp) (lines 353-354)
- _lockDuration < lockdrop.minLockDuration || _lockDuration > uint32(configStorage.getUint(StorageKey.MaxLockDuration)) (lines 357-359)
- nftOverlord.addReveal(_lockRecipient,uint16(numberNFTs)) (line 369)
- lockedToken.lastLockTime = uint32(block.timestamp) (line 381)
- lockedToken.unlockTime = uint32(block.timestamp) + uint32(_lockDuration) (lines 382-384)
proposeUSDPrice Function (src/managers/LockManager.sol lines 142-174):
- usdUpdateProposal.proposedDate = uint32(block.timestamp) (line 166)
configureLockDrop Function (src/managers/LockManager.sol lines 98-112):
- revert LockdropEndedError(uint32,uint32)(_lockdropData.end,uint32(block.timestamp)) (lines 102-105)
unlock Function (src/managers/LockManager.sol lines 401-427):
- lockedToken.unlockTime > uint32(block.timestamp) (line 410)

Detailed Explanation:

setLockDuration Function:
- The function sets the lock duration for a player. The duration is provided as a uint256 but is cast to a uint32 without checking if the value exceeds the maximum limit of uint32 (2^32 - 1), leading to potential truncation and incorrect lock durations.
_lock Function:
- Similar to setLockDuration, the _lock function handles lock durations and timestamps. The conversion from uint256 to uint32 can result in incorrect lock times if the provided values exceed the uint32 limit.
proposeUSDPrice Function:
- This function proposes a new USD price for tokens. The proposed price is a uint256, but the function does not validate the size before using it, which can lead to unexpected behavior if the value is too large.
configureLockDrop Function:
- The function configures the lockdrop with start and end times. These times are provided as uint256 but are cast to uint32 without validation, potentially leading to incorrect configurations.
unlock Function:
- The function unlocks tokens for a player. The quantity to unlock is a uint256, but the function does not validate the size before using it, which can lead to unexpected behavior if the value is too large.

Potential Impact:

Incorrect Lock Durations: Players may experience incorrect lock durations, leading to premature or delayed unlocking of tokens.
Unexpected Behavior: The contract may behave unpredictably due to the truncation of large values, potentially causing financial losses or denial of service.
Security Risks: Attackers may exploit these typecasting issues to manipulate lock durations, token quantities, or other critical parameters, leading to potential financial and operational risks.

Recommendations:

Validation: Implement proper validation checks to ensure that values being cast from uint256 to uint32 do not exceed the maximum limit of uint32.
Safe Typecasting: Use safe typecasting libraries or techniques to handle conversions and prevent truncation or overflow issues.
Testing: Thoroughly test the contract with edge cases to ensure that typecasting does not lead to unexpected behavior or vulnerabilities.

By addressing these issues, we can enhance the security and reliability of the LockManager contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.25;

import "forge-std/Test.sol";
import "../src/managers/LockManager.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract LockManagerTest is Test {
    LockManager lockManager;
    ERC20 token;
    address admin = address(0x1);
    address user = address(0x2);

    function setUp() public {
        token = new ERC20("Test Token", "TTK");
        lockManager = new LockManager(address(this));
        lockManager.configureToken(address(token), ConfiguredToken({
            nftCost: 1e18,
            active: true,
            decimals: 18,
            usdPrice: 1e18
        }));
        vm.startPrank(admin);
        lockManager.configureLockdrop(Lockdrop({
            start: uint32(block.timestamp),
            end: uint32(block.timestamp + 1 days),
            minLockDuration: 1 days
        }));
        vm.stopPrank();
    }

    function testSetLockDurationOverflow() public {
        vm.startPrank(user);
        uint256 overflowDuration = type(uint256).max;
        lockManager.setLockDuration(overflowDuration);
        PlayerSettings memory settings = lockManager.getPlayerSettings(user);
        assertEq(settings.lockDuration, uint32(overflowDuration));
        vm.stopPrank();
    }

    function testLockOverflow() public {
        vm.startPrank(user);
        uint256 overflowDuration = type(uint256).max;
        token.approve(address(lockManager), 1e18);
        lockManager.setLockDuration(overflowDuration);
        lockManager.lock(address(token), 1e18);
        LockedTokenWithMetadata[] memory lockedTokens = lockManager.getLocked(user);
        assertEq(lockedTokens[0].lockedToken.unlockTime, uint32(block.timestamp) + uint32(overflowDuration));
        vm.stopPrank();
    }

    function testProposeUSDPriceOverflow() public {
        vm.startPrank(admin);
        uint256 overflowPrice = type(uint256).max;
        address[] memory contracts = new address[](1);
        contracts[0] = address(token);
        lockManager.proposeUSDPrice(overflowPrice, contracts);
        // Check if the proposed price is set correctly
        // This will not revert but will show the vulnerability
        vm.stopPrank();
    }

    function testConfigureLockDropOverflow() public {
        vm.startPrank(admin);
        uint256 overflowEnd = type(uint256).max;
        lockManager.configureLockdrop(Lockdrop({
            start: uint32(block.timestamp),
            end: uint32(overflowEnd),
            minLockDuration: 1 days
        }));
        // Check if the lockdrop end time is set correctly
        // This will not revert but will show the vulnerability
        vm.stopPrank();
    }

    function testUnlockOverflow() public {
        vm.startPrank(user);
        uint256 overflowDuration = type(uint256).max;
        token.approve(address(lockManager), 1e18);
        lockManager.setLockDuration(overflowDuration);
        lockManager.lock(address(token), 1e18);
        vm.warp(block.timestamp + 2 days); // Fast forward time to ensure unlock time has passed
        lockManager.unlock(address(token), 1e18);
        LockedTokenWithMetadata[] memory lockedTokens = lockManager.getLocked(user);
        assertEq(lockedTokens[0].lockedToken.quantity, 0);
        vm.stopPrank();
    }
}

Tools Used

Slither.

Recommended Mitigation Steps

Recommendation: Addressing Typecasting Vulnerabilities in LockManager Functions

Description:

The identified vulnerability stems from improper typecasting of variables from uint256 to uint32 without adequate validation. This can lead to truncation and unexpected behavior. The issue affects several functions in the LockManager contract, including setLockDuration, _lock, proposeUSDPrice, configureLockDrop, and unlock.

Steps to Resolve:

Implement Validation Checks: Ensure that values cast from uint256 to uint32 do not exceed the maximum limit of uint32 (2^32 - 1). If the value exceeds this limit, revert the transaction with an appropriate error message.
Use Safe Typecasting Libraries: Utilize libraries like OpenZeppelin's SafeCast to handle safe typecasting and prevent truncation or overflow issues.
Update Functions with Validation: Modify the affected functions to include validation checks before typecasting.

Code Changes:

setLockDuration Function:

function setLockDuration(uint256 _duration) external notPaused {
   uint256 maxLockDuration = configStorage.getUint(StorageKey.MaxLockDuration);
   if (_duration > maxLockDuration) revert MaximumLockDurationError();
   if (_duration > type(uint32).max) revert DurationExceedsUint32Error();

   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) {
           if (block.timestamp + _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);
}

_lock Function:

function _lock(
   address _tokenContract,
   uint256 _quantity,
   address _tokenOwner,
   address _lockRecipient
) private {
   // ... existing code ...

   uint256 maxLockDuration = configStorage.getUint(StorageKey.MaxLockDuration);
   if (_lockDuration > maxLockDuration) revert MaximumLockDurationError();
   if (_lockDuration > type(uint32).max) revert DurationExceedsUint32Error();

   // ... existing code ...

   lockedToken.lastLockTime = uint32(block.timestamp);
   lockedToken.unlockTime = uint32(block.timestamp) + uint32(_lockDuration);

   // ... existing code ...
}

proposeUSDPrice Function:

function proposeUSDPrice(
   uint256 _price,
   address[] calldata _contracts
) external onlyOneOfRoles([Role.PriceFeed_1, Role.PriceFeed_2, Role.PriceFeed_3, Role.PriceFeed_4, Role.PriceFeed_5]) {
   // ... existing code ...

   usdUpdateProposal.proposedDate = uint32(block.timestamp);

   // ... existing code ...
}

configureLockDrop Function:

function configureLockdrop(
   Lockdrop calldata _lockdropData
) external onlyAdmin {
   if (_lockdropData.end < block.timestamp) revert LockdropEndedError(_lockdropData.end, uint32(block.timestamp));
   if (_lockdropData.start >= _lockdropData.end) revert LockdropInvalidError();

   lockdrop = _lockdropData;

   emit LockDropConfigured(_lockdropData);
}

unlock Function:

function unlock(
   address _tokenContract,
   uint256 _quantity
) external notPaused nonReentrant {
   LockedToken storage lockedToken = lockedTokens[msg.sender][_tokenContract];
   if (lockedToken.quantity < _quantity) revert InsufficientLockAmountError();
   if (lockedToken.unlockTime > uint32(block.timestamp)) revert TokenStillLockedError();

   // ... existing code ...
}

Conclusion:

By implementing these validation checks and utilizing safe typecasting techniques, we can prevent truncation and overflow issues, thereby enhancing the security and reliability of the LockManager contract.

Assessed type

Context

code-423n4 / 2024-05-munchables-validation

Dubious typecast in the functions called setLockDuration & _lock proposeUSDPrice & configureLockDrop and unlock function #535

Lines of code

Vulnerability details

Impact

Vulnerability: Dubious Typecasting in LockManager Functions

Description:

Affected Locations:

Detailed Explanation:

Potential Impact:

Recommendations:

Proof of Concept

Tools Used

Recommended Mitigation Steps

Recommendation: Addressing Typecasting Vulnerabilities in LockManager Functions

Description:

Steps to Resolve:

Code Changes:

Conclusion:

Assessed type