Use of payable.transfer and inability to specify a recipient will cause tokens to be locked forever if unlocker is a contract or multisig wallet

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

The unlock function allows users to unlock only for themselves to themselves i.e the unlocked tokens are on msg.sender's account and transferred to msg.sender. This is very important as when trying to unlock their tokens, the .transfer method is used if tokenContract is address(0). The transfer method only forwards 2300 gas, which is not enough for the recipient to execute any non-trivial logic in a receive() or fallback function. If the locker is a contract or a multisig wallet, the gas forwarded is not going to be enough, causing that the unlock function will always revert. Now, since there's no option to provide a recipient, the user will permanently not be able to withdraw his tokens.

Proof of Concept

Compared to the 4naly3er report which only discusses how the call may fail, here in this case, there is a notable absence of the ability to specify a recipient when unlocking which means a user can only claim for himself. This makes the severity much higher as whenever the user calls the unlock function from his contract account like a multisig or smart contract wallet that has a receive function requiring >2300 gas, the call will always fail, and as there's no way to specify a different recipient, the tokens will be locked in the the contract forever, leading to loss of funds.

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

        // force harvest to make sure that they get the schnibbles that they are entitled to
        accountManager.forceHarvest(msg.sender);

        lockedToken.quantity -= _quantity;

        // send token
        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);
        } else {
            IERC20 token = IERC20(_tokenContract);
            token.transfer(msg.sender, _quantity);
        }

        emit Unlocked(msg.sender, _tokenContract, _quantity);
    }

Tools Used

Manual code review

Recommended Mitigation Steps

Use call instead of transfer to send ETH instead as it forwards all available gas and allows contract recipients to execute arbitrary logic.

        if (_tokenContract == address(0)) {
            payable(msg.sender).transfer(_quantity);

Or introduce the ability to specify recipient when unlocking. The tokens can be sent to the recipient instead.

Assessed type

ETH-Transfer

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope

[alex-ppg]

Hey @ZanyBonzy, thanks for your due diligence! As the judge's response on the thread shared states, the SC ruling is consultative and not authoritative. Additionally, it is up to the preference of the judge whether to accept such findings as valid.

IMO, a finding that is upgraded by a single severity tier is not sufficient to be considered a valid submission. An M and H risk issue is equivalent in urgency for the sponsor as it leads to an impact that needs to be resolved in the code. Should this have been a QA (L) issue that was upgraded to H as this particular case I personally judged, it would have been treated differently.

My initial ruling remains and I believe it is fair when it comes to the effort invested in post-processing automated findings.