Revert Lend - Tigerfrake's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src%2Ftransformers%2FV3Utils.sol#L115

Vulnerability details

Summary

The V3Utils::execute() function facilitates token swapping, liquidity management, fee collection, and other operations related to liquidity provision.

The vulnerability arises from the missing check that ensures the caller of the execute() function is the owner of the token associated with the tokenId. This means that anyone can call the execute() function and perform operations on behalf of any token holder without proper authorization.

Proof of Concept

• https://github.com/code-423n4/2024-03-revert-lend/blob/main/src%2Ftransformers%2FV3Utils.sol#L115

    function execute(uint256 tokenId, Instructions memory instructions) public returns (uint256 newTokenId) {

When execute() is called in executeWithPermit() in the line return execute(tokenId, instructions); it performs the execution on the given tokenId with proper access control. However, he execute() function is marked as public, which means it can be called directly by any external account or contract.

This opens up the possibility for any caller to execute instructions on any arbitrary tokenId, bypassing the access control check performed in the executeWithPermit() function.

This could potentially lead to unauthorized execution of instructions on tokens that the caller should not have access to, violating the intended access control mechanism of the contract.

Exploit scenario:

Without this check:

• An attacker could call this function and perform actions on liquidity positions owned by other users, potentially manipulating their positions or stealing their assets.
• This could lead to unauthorized transfers, loss of funds, or manipulation of the exchange's liquidity.

Tools Used

Manual review

Recommended Mitigation Steps

Adding the ownership check ensures that only the rightful owner of the token can execute operations on their own liquidity positions.

Add this check to the execute() function.

        if (nonfungiblePositionManager.ownerOf(tokenId) != msg.sender) {
            revert Unauthorized();
        }

OR

To address this issue, you should consider making the execute function internal or private if it's only intended to be called internally within the contract. By making it internal or private, you ensure that it can only be invoked by other functions within the contract and not directly by external callers.

Here's how you can modify the function declaration:

function execute(uint256 tokenId, Instructions memory instructions) internal returns (uint256 newTokenId) {
    // Function implementation remains unchanged
}

Assessed type

Access Control