Lines of code

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

Vulnerability details

Impact

The protocol supports rebasing token behavior in scope but the LockManager.sol contract doesn't implement any functionality for handling rebasing tokens. This leads to rebasing rewards getting stuck on the contract forever or users may be unable to withdraw any tokens if rebasing tokens are deposited.

Proof of Concept

The contest readme clearly mentions Balance changes outside of transfers behaviour is in scope. Thus, rebasing/airdrop tokens are supported by the protocol.

When the lock() function is called to lock tokens inside the contract, the balance is cached as it is.

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

        // Transfer erc tokens
        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
            token.transferFrom(_tokenOwner, address(this), _quantity);
        }

        ...

        lockedToken.quantity += _quantity;

In case of rebasing(token balance goes up/down without transfers), It is possible that the contract balances decreases over time but user can withdraw their deposit balances using the unlock() function without accounting for the decreased amount. This results in other users funds getting cut off or user being unable to withdraw any amount.

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

        lockedToken.quantity -= _quantity;

        ...

        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }
        ...

    }

It is also possible that the balances will increase over time but the users can only withdraw the deposit amount and the rest of the increased balance will be stuck in the contract forever as there is no way to recover those increased amounts.

Tools Used

Manual Analysis

Recommended Mitigation Steps

For rebasing tokens, when they go down in value, there should be a method to update the cached reserves accordingly, based on the balance held. This is a complex solution. Also, when they go up in value, there should be a method to actually transfer the excess tokens out of the protocol (possibly directly to users).

Assessed type

Token-Transfer

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

Rebasing rewards will get stuck on the contract forever or users may be unable to withdraw any tokens if rebasing tokens are deposited. #433