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

8 stars 6 forks source link

Missing access controls in mint functions allows anyone to create unlimited position nfts #280

Closed c4-bot-1 closed 3 months ago

c4-bot-1 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PositionNFTs.sol#L142-L149 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PositionNFTs.sol#L174-L191

Vulnerability details

Bug Description

mintPosition() and mintPositionForUser(address) of PositionNFTs contract do not validate the authority of the minter, allowing anyone to mint unlimited position nfts.

 function mintPosition()
        external
        returns (uint256)
    {
        return _mintPositionForUser(
            msg.sender
        );
    }
function mintPositionForUser(
        address _user
    )
        external
        returns (uint256)
    {
        if (isApprovedForAll(
                _user,
                msg.sender
            ) == false
        ) {
            revert NotPermitted();
        }

        return _mintPositionForUser(
            _user
        );
    }

The attacker can mint unlimited position nfts and then call withdraw funcitions in WiseLending contract and steal raw ETH from the protocol

Impact

Anyone can mint unlimited position nfts as a result they can then directly steal ETH from wiselending by calling ETH withdrawal functions.

Proof of Concept

Since the bug is straightforward i think POC isn't required

Tools Used

Manual Analysis

Recommended Mitigation Steps

Create a minter role and enforce that only minter can mint position nfts for users

function mintPositionForUser(address _user) external onlyMinter returns (uint256) {
  ... ... ...
}

Assessed type

Access Control

vm06007 commented 4 months ago

This is not a high, nor it is a relevant find. the code is considered to work as expected, modifier onlyReserverRole should only control ability to reserve for other addresses, initial public reservation for self address should happen as user see fit and should be able to reserve only 1 NFT per address (which is the case now)

no additional changes are necessary as it is absolutely fine if people want to mint more NFTs per address with reservation capabilities.

absolutely untrue claims Anyone can mint unlimited position nfts as a result they can then directly steal ETH from wiselending by calling ETH withdrawal functions.

1) anyone should be able to mint as much as they like, if user MINTs NFT it does not steal any ETH, these NFTs represent empty positions that users can use to supply and borrow funds per ownership of the NFT, 2) there is no way to steal any ETH if user simply mints NFT (please consider to understand codebase better and what does NFT role is suppose to mint

if you still disagree provide example how ETH would be stolen, otherwise marking this as invalid 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