code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L713-L723

Vulnerability details

Impact

withdrawFees don't check that to address is not zero and send fee to the address without any check that confirms admin has set the address. (bentoBox don't accept transferring to zero address, otherwise this could be high risk)

Proof of Concept

As you can see in the code, there is no check for to address, and code don't check to see that if it's already initialized and after that transfer assets. event so bentoBox checks for the zero address but it will be good practice to check that here too, In case logics in bentoBox changes.

    /// @notice Withdraws the fees accumulated.
    function withdrawFees() public {
        address to = masterContract.feeTo();

        uint256 _share = feesEarnedShare;
        if (_share > 0) {
            bentoBox.transfer(asset, address(this), to, _share);
            feesEarnedShare = 0;
        }

        emit LogWithdrawFees(to, _share);
    }

Tools Used

VIM

Recommended Mitigation Steps

add this line to withdrawFees(): require(address(to) != address(0), "NFTPair: address is zero");

cryptolyndon commented 2 years ago

Not a bug, for a reason that is even in the report: the BentoBox would prevent this from going through.

I would argue that is better practice to know things like this, than to put checks in "just in case" you don't understand a major piece of your own infrastructure.

0xean commented 2 years ago

Closing as invalid.

code-423n4 / 2022-04-abranft-findings