c4-bot-6 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PowerFarms/PowerFarmNFTs/MinterReserver.sol#L96-L113 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PositionNFTs.sol#L222-L243

Vulnerability details

Impact

The equivalence in the isOwner function's true conditions within the MinterReserver and PositionNFTs contracts may inadvertently lead to an expansion of permissions.

Proof of Concept

The isOwner function in the MinterReserver.sol verifies ownership through two checks:

Checking for reserved key ownership in the reservedKeys mapping.
Verifying NFT ownership via the FARMS_NFTS.ownerOf function.

These two checks are considered equivalent for verify ownership, based on the following assumptions:

Both reserved status and NFT ownership suffice to prove a user's control over a key.
For simplified logic

In MinterReserver.sol:

function isOwner(uint256 _keyId, address _owner) public view returns (bool) {
    if (reservedKeys[_owner] == _keyId) {
        return true;
    }
    if (FARMS_NFTS.ownerOf(_keyId) == _owner) {
        return true;
    }
    return false;
}

The isOwner function in PositionNFTs.sol follows a similar design philosophy, where multiple conditions are treated as equivalent for ownership verification.

In PositionNFTs.sol:

function isOwner(uint256 _nftId, address _owner) external view returns (bool) {
    if (_nftId == FEE_MANAGER_NFT) {
        return feeManager == _owner;
    }
    if (reserved[_owner] == _nftId) {
        return true;
    }
    if (ownerOf(_nftId) == _owner) {
        return true;
    }
    return false;
}

This approach is based on the premise that these conditions are strictly equivalent. However, should there be changes in business processes or the definitions of these conditions in the future, this design could lead to potential risks as outlined below:

Overly Broad Access: Reserved keys, intended for temporary access, might inadvertently grant the same level of control as permanent NFT ownership. This could allow users to execute actions meant only for full NFT owners.
State Discrepancies: The temporary nature of reserved keys could lead to state mismatches, especially if not transitioned properly to NFT ownership. This might result in users simultaneously possessing both a reserved key and its corresponding NFT, conflicting with intended contract logic.
Logical Confusion: Treating temporary reserved keys and permanent NFT ownership as equivalent can complicate contract logic, heightening the risk of vulnerabilities and complicating security audits.
Bypassed Security Checks: There's a risk that users might exploit the reservation mechanism to bypass critical security checks meant for NFT owners, especially in processes like NFT minting.

Tools Used

Manual

Recommended Mitigation Steps

Documentation and Transparency: Ensure comprehensive documentation of ownership states and their implications on access rights within the contract, aiding both understanding and auditing efforts.
Clarify Ownership Levels: Clearly define the levels of ownership within the contract, distinguishing between the temporary nature of reserved keys and the permanence of NFT ownership.
Enhanced Access Control: Implement access controls that respect the distinct levels of ownership, ensuring that only permanent owners can access functionalities critical to contract integrity.
State Transition Management: Develop mechanisms to manage the transition from temporary to permanent ownership, ensuring that temporary owners cannot exploit their status beyond intended use.