Unauthorized token relocking in _withdrawExpiredLocksFor allows fund locking without user consent

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/reward/MultiFeeDistribution.sol#L1351-L1381

Vulnerability details

Impact

DOS. The implementation of the _withdrawExpiredLocksFor function allows anybody to relock expired tokens on behalf of another user without their explicit permission. This leads to unexpected token locking preventing users from accessing their funds when they intend to.

Proof of Concept

Look at the _withdrawExpiredLocksFor function: https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/reward/MultiFeeDistribution.sol#L1351-L1381

function _withdrawExpiredLocksFor(
        address address_,
        bool isRelockAction,
        bool doTransfer,
        uint256 limit
    ) internal whenNotPaused returns (uint256 amount) {
        if (isRelockAction && address_ != msg.sender && _lockZap != msg.sender) revert InsufficientPermission();
        _updateReward(address_);

        uint256 amountWithMultiplier;
        Balances storage bal = _balances[address_];
        (amount, amountWithMultiplier) = _cleanWithdrawableLocks(address_, limit);
        bal.locked = bal.locked - amount;
        bal.lockedWithMultiplier = bal.lockedWithMultiplier - amountWithMultiplier;
        bal.total = bal.total - amount;
        lockedSupply = lockedSupply - amount;
        lockedSupplyWithMultiplier = lockedSupplyWithMultiplier - amountWithMultiplier;

        if (isRelockAction || (address_ != msg.sender && !autoRelockDisabled[address_])) {     <-------@
            _stake(amount, address_, defaultLockIndex[address_], true);
        } else {
            if (doTransfer) {
                IERC20(stakingToken).safeTransfer(address_, amount);
                incentivesController.afterLockUpdate(address_);
                emit Withdrawn(address_, amount, _balances[address_].locked, 0, 0, stakingToken != address(rdntToken));
            } else {
                revert InvalidAction();
            }
        }
        return amount;
    }

The condition (address_ != msg.sender && !autoRelockDisabled[address_]) allows any user to relock tokens for another user if auto-relocking is not disabled. This can happen even if isRelockAction is false, which means it wasn't explicitly intended to be a relocking action.

An attacker could exploit this by calling the function with isRelockAction set to false, doTransfer set to false, and targeting a user who hasn't disabled auto-relocking. This would cause the tokens to be relocked without the owner's consent.

Tools Used

Manual code review

Recommended Mitigation Steps

Modify the condition to only allow relocking if it's an explicit relock action and the caller is either the token owner or the authorized _lockZap contract. This is to ensure that relocking can only occur when explicitly requested (isRelockAction is true) and only by the token owner or the authorized _lockZap contract thus preventing unauthorized relocking of tokens, protecting users from unexpected fund locking.

function _withdrawExpiredLocksFor(
    address address_,
    bool isRelockAction,
    bool doTransfer,
    uint256 limit
) internal whenNotPaused returns (uint256 amount) {
    if (isRelockAction && address_ != msg.sender && _lockZap != msg.sender) revert InsufficientPermission();
    _updateReward(address_);

    // ... (code omitted for brevity)

-   if (isRelockAction || (address_ != msg.sender && !autoRelockDisabled[address_])) {
+   if (isRelockAction && (address_ == msg.sender || msg.sender == _lockZap)) {
        _stake(amount, address_, defaultLockIndex[address_], true);
    } else {
        if (doTransfer) {
            IERC20(stakingToken).safeTransfer(address_, amount);
            incentivesController.afterLockUpdate(address_);
            emit Withdrawn(address_, amount, _balances[address_].locked, 0, 0, stakingToken != address(rdntToken));
        } else {
            revert InvalidAction();
        }
    }
    return amount;
}

Assessed type

DoS

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)