Munchables - zhaojohnson's results

Lines of code

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

Vulnerability details

Impact

Users' unlock time can be extended by hackers. This will lead to user's funds locked in the contract forever.

Proof of Concept

In lockOnBehalf(), the hacker can make use of his own funds to lock for the victim. And after that, the victim's unlock time will be extended based on current time. Attack vector is like: the hacker call lockOnBehalf with 0 _quantity or a very small amount, the victim _onBehalfOf's unlock time will be extended. Considering the minimum LockDuration is 30 days, it's easy to lock the victims' funds forever.

    function lockOnBehalf(
        address _tokenContract,
        uint256 _quantity,
        address _onBehalfOf
    )
        external
        payable
        notPaused
        onlyActiveToken(_tokenContract)
        onlyConfiguredToken(_tokenContract)
        nonReentrant
    {
        address tokenOwner = msg.sender;
        address lockRecipient = msg.sender;
        if (_onBehalfOf != address(0)) {
            lockRecipient = _onBehalfOf;
        }

        _lock(_tokenContract, _quantity, tokenOwner, lockRecipient);
    }

    function _lock(
        address _tokenContract,
        uint256 _quantity,
        address _tokenOwner,
        address _lockRecipient
    ) private {
        ......
        LockedToken storage lockedToken = lockedTokens[_lockRecipient][
            _tokenContract
        ];
        // Transfer erc tokens
        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }
        lockedToken.remainder = remainder;
        lockedToken.quantity += _quantity;
        lockedToken.lastLockTime = uint32(block.timestamp);
@==> unlock time will be extended by lockDuration based on current timestamp.
        lockedToken.unlockTime =
            uint32(block.timestamp) +
            uint32(_lockDuration);

        // set their lock duration in playerSettings
        playerSettings[_lockRecipient].lockDuration = _lockDuration;

    }

Tools Used

Manual

Recommended Mitigation Steps

Don't allow the lockOnBehalf or add one whitelist for the user. Only approved address by the user can call lockOnBehalf().

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Users' lock time can be reduced because incorrect time lock set in setLockDuration.

Proof of Concept

Users can call function setLockDuration() to update lockedTokens' lock time. The function has already checked uint32(block.timestamp) + uint32(_duration) < lockedTokens[msg.sender][tokenContract].unlockTime. It means that the update unlockTime should be equal to or larger than the previous unlockTime. The vulnerability is the incorrect calculation of unlockTime

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

The actual unlock time should be block.timestamp + _duariont.

For example:

• Alice locks tokens in timestamp A, and the unlock time is A + 30, assume the duration is 30 Days.
• Alice calls setLockDuration to update unlock time when 15 days pass. Now the updated unlock time is A + 15. It's less than the previous unlock time. And users can unlock their funds directly.

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 (
                    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);
            }
        }
    }

Tools Used

Manual

Recommended Mitigation Steps

Change the calculation of unlockTime in setLockDuration.

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

Assessed type

Context