code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-canto-identity/blob/dff8e74c54471f5f3b84c217848234d474477d82/src/CidNFT.sol#L147 https://github.com/code-423n4/2023-01-canto-identity/blob/dff8e74c54471f5f3b84c217848234d474477d82/src/CidNFT.sol#L150

Vulnerability details

Impact

Detailed description of the impact of this finding. It is possible to call mint(bytes[] calldata _addList) with the some data. This data will be used to delegatecall add function to add subprotocol NFTs directly after minting. There is no check on length or gas check. There might be out of gas.

    function mint(bytes[] calldata _addList) external {
        _mint(msg.sender, ++numMinted); // We do not use _safeMint here on purpose. If a contract calls this method, he expects to get an NFT back
        bytes4 addSelector = this.add.selector;
        for (uint256 i = 0; i < _addList.length; ++i) {
            (
                bool success, /*bytes memory result*/

            ) = address(this).delegatecall(abi.encodePacked(addSelector, _addList[i]));
            if (!success) revert AddCallAfterMintingFailed(i);
        }
    }

Proof of Concept

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

For example I want to mint NFT and add as much as i want (as a marketplace there is new collection and my contract add it in butches, size of butch ~10000(as example)) Each add operation contains (in worst case):

transfer erc721
2 transfer for fee
calling remove function
writing to storage variable(active type) (changing old)
erc721 transfer(that's call reciever function in another contract with all of gas)
one more writing to storage (push new)

And that all can cost a lot of gas and easily can lose all gas sended with it, but change nothing in storage.

Tools Used

Foundry for testing

Recommended Mitigation Steps

set limit for size of _addList(then you need to limit name of subprotocol)
set limit for elements in _addlist (as example 10 or 100 it depends)
check on each iterations how much gas left and set the border. Next user to client number of added NfT's
check gas at start of function, but it(gas cost for function) possibly can be changed with one of next forks

berndartmueller commented 1 year ago

Not a systematic risk as this represents a user error. Downgrading to QA (NC).

c4-judge commented 1 year ago

berndartmueller changed the severity to QA (Quality Assurance)