c4-bot-8 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PositionNFTs.sol#L90-L97 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PositionNFTs.sol#L99-L109

Vulnerability details

Bug Description

In order to understand this bug, we need to have overall context of positionNFT contract. Here we go:

    mapping(address => bool) public reserveRole;
    bool public reservePublicBlocked;

These two states are crucial to understand this bug. reserveRole stores addresses of users who have the authority to reserve nfts even after public reserving is blocked. While as reservePublicBlocked tracks whether public reserving is blocked or not.

Apart from these two states, onlyReserveRole() modifier checks if the public reserves are blocked, when they are it further validates whether the caller is assigned reserving responsibilities

    modifier onlyReserveRole() {
        if (reservePublicBlocked == true) {
            if (reserveRole[msg.sender] == false) {
                revert NotPermitted();
            }
        }
        _;
    }

reservePositionForUser() implements this modifier and functions as expected without introducing any bugs.

    function reservePositionForUser(
        address _user
    )
        onlyReserveRole
        external
        returns (uint256)
    {
        return _reservePositionForUser(
            _user
        );
    }

But unfortunately there is another function that allows anyone to reserve nfts regardless of reserveRole

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PositionNFTs.sol#L90-L97

    function reservePosition()
        external
        returns (uint256)
    {
        return _reservePositionForUser(
            msg.sender
        );
    }

The user can then mint the reserved nft using either approveMint(address,uint256), mintPositionForUser(address) or even with mintPosition()

Impact

The PositionNfts Contract is supposed to allow non reserveRoles to reserve nfts only when reservePublicBlocked is false, but due to the lack of proper access controls, anyone can reserve nfts even after reservePublicBlocked is set to true.

For the further impact, user can then mint the reserved nft and withdraw it for ETH in Wiselending contract

Proof of Concept

Please note that, due to depreciation of _exists function in openzeppelin contracts, PositionNFT.sol causes compilation issues, so in order to run this POC comment, these lines out:

import "forge-std/Test.sol";
import "contracts/PositionNFTs.sol";

contract PositionNFTtest is Test {
    PositionNFTs nft;
    address deployer = makeAddr("deployer");
    address nftFeeManager = makeAddr("feemanager");
    address reserver1 = makeAddr("reserver1");
    address alice = makeAddr("alice");
    address bob = makeAddr("bob");

    function setUp() external {
        // deploy the nft with deployer's address so that you know who the OwnableMaster is!
        vm.startPrank(deployer);
        nft = new PositionNFTs("name", "symbol", "baseUri");
        vm.stopPrank();
    }

//@audit-issue when public position nfts are blocked, user can still reserve positon via reservePostion() with no access-controls
// bug worse they can then mint with either mintPos()'s or even approveMint()
    function test_reservingNft() public {
        console.log("reservePublicBlocked_before: ");
        console.logBool(nft.reservePublicBlocked());

        // block reserves
        vm.startPrank(deployer);
        nft.blockReservePublic();
        vm.stopPrank();

        // after blockage status logging
        console.log("reservePublicBlocked_after: ");
        console.logBool(nft.reservePublicBlocked());

        // alice reserves position
        vm.startPrank(alice);
        // nft.reservePositionForUser(alice); // reverts when public reserves are blocked
        uint ReservedId = nft.reservePosition(); //@audit-issue bypass for blockage of public reserves
        vm.stopPrank();

        // Log alice's reserves
        console.log("reserved of alice: ", nft.reserved(alice));
        console.log("alice is now minting: ");
        vm.startPrank(alice);
        uint tokenId = nft.mintPosition();
        vm.stopPrank();
        console.log(
            "reserved-ownership: ",
            nft.reserved(alice),
            nft.ownerOf(tokenId)
        );
        console.log("alice :=", alice);
    }
}

Tools Used

Manual Analysis, Foundry

Recommended Mitigation Steps

Implement onlyReserveRole() modifier on reservePosition() function

    function reservePosition()
        external 
+       onlyReserveRole
        returns (uint256)
    {
        return _reservePositionForUser(
            msg.sender
        );
    }

Assessed type

Access Control

vm06007 commented 4 months ago

This is invalid submission.

Users should ALWAYS be able to reserve NFTs as long as they reserve for themselves, this is desired functionality. The author of this "finding" does not understand that reservePublicBlocked is only applied on ability to reserve NFTs for other users (hence when someone else reserves NFTs for other arbitrary address) but as long as user performs reservation for their own account this is fine. Team specifically need to separate from ability to reserve for self and other users. If the suggested code change is applied that author of this "finding" suggests, then it is not possible to differentiate and both functions are blocked at the same time.

This modifier should only be applied for one function as a way to control who can use reservePositionForUser Normal reservations reservePosition should always be possible. "Finding" dismissed.

c4-pre-sort commented 4 months ago

GalloDaSballo marked the issue as insufficient quality report

c4-judge commented 3 months ago

trust1995 marked the issue as unsatisfactory: Invalid

code-423n4 / 2024-02-wise-lending-findings

Non-reserveRoles can still reserve positonNFTs even after public reserves are blocked, due to inconsistent access controls #291