Lines of code

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

Vulnerability details

Impact

Incorrect assignment of the tokenOwner variable can cause the contract to attempt to transfer tokens from the wrong address, potentially leading to failed transactions or unauthorized token transfers.

In the current implementation, the tokenOwner is always assigned the value of msg.sender, which is the address that called the lockOnBehalf function. However, when locking tokens on behalf of another address (_onBehalfOf), the tokenOwner should be the address that approved the token transfer, not the address calling the lockOnBehalf function.

Example scenario where this bug could lead to unintended behavior.

Alice approves Bob to spend 100 tokens on her behalf. Charlie calls the lockOnBehalf function, specifying Alice's address as _onBehalfOf. The tokenOwner is set to Charlie's address (the caller), instead of Alice's address (the token owner). The contract attempts to transfer tokens from Charlie's balance, which may fail if Charlie doesn't have enough tokens approved.

Proof of Concept

Let's consider a scenario where Alice wants to lock tokens on behalf of Bob using the lockOnBehalf function.

Alice approves the LockManager contract to spend 100 tokens on her behalf:

// Assuming Alice has 100 tokens in her balance
erc20Token.approve(lockManagerContract, 100);

Alice calls the lockOnBehalf function, specifying Bob's address as the _onBehalfOf parameter.

lockManagerContract.lockOnBehalf(erc20Token, 100, bob);

In the current implementation of lockOnBehalf, the tokenOwner is incorrectly assigned to msg.sender, which is Alice's address: LockManager.sol#L275-L293

function lockOnBehalf(
    address _tokenContract,
    uint256 _quantity,
    address _onBehalfOf
)
    external
    payable
    notPaused
    onlyActiveToken(_tokenContract)
    onlyConfiguredToken(_tokenContract)
    nonReentrant
{
    address tokenOwner = msg.sender; // Incorrect assignment
    address lockRecipient = _onBehalfOf != address(0) ? _onBehalfOf : msg.sender;

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

The _lock function is called with Alice's address as the tokenOwner, even though the tokens should be transferred from Bob's balance (since Alice approved the transfer on Bob's behalf).

If Bob doesn't have enough tokens approved for the LockManager contract, the transaction will fail, even though Alice has approved the required amount.

Tools Used

Manual Review

Recommended Mitigation Steps

The tokenOwner should be assigned the value of _onBehalfOf when it's not the zero address.

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

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

Assessed type

Error

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

Incorrect Assignment of tokenOwner Variable in lockOnBehalf Function #335