code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L454 https://github.com/code-423n4/2022-06-nibbl/blob/main/contracts/NibblVault.sol#L474

Vulnerability details

Impact

Detailed description of the impact of this finding.

I noticed that the withdrawUnsettledBids() and redeem() functions return the Ether amount by calling safeTransferETH, but if the to address passed in is a malicious contract address and the receive() function is implemented inside the contract and revert() is called inside receive. Therefore the transaction will be rolled back. This will lead to some degree of denial of service, so I think we should define a function where the to address acts as a caller to actively claim the Ether amount instead of actively issuing it for it.

    function withdrawUnsettledBids(address payable _to) external override {
        uint _amount = unsettledBids[msg.sender];
        delete unsettledBids[msg.sender];
        totalUnsettledBids -= _amount;
        safeTransferETH(_to, _amount);
    }

    function redeem(address payable _to) external override boughtOut returns(uint256 _amtOut){
        uint256 _balance = balanceOf(msg.sender);
        _amtOut = ((address(this).balance - feeAccruedCurator - totalUnsettledBids) * _balance) / totalSupply();
        _burn(msg.sender, _balance);
        safeTransferETH(_to, _amtOut);
    }

    function safeTransferETH(address payable _to, uint256 _amount) private {
        (bool success, ) = _to.call{value: _amount}("");
        require(success, "NibblVault: ETH transfer failed");
    }

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

I think we should define a function where the to address acts as a caller to actively claim the Ether amount instead of actively issuing it for it.

GalloDaSballo commented 2 years ago

Finding is saying that caller can DOS themselves, at their own expense, to no impact to anyone else

HardlyDifficult commented 2 years ago

Finding is saying that caller can DOS themselves, at their own expense, to no impact to anyone else

Agree. I'm going to close this as invalid.

The warden is not wrong about the potential but as was pointed out here - the address is an input field for the function being called.

code-423n4 / 2022-06-nibbl-findings