search

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

1 stars 0 forks source link

Basket NFT have no name and symbol #317

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L13 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L6

Vulnerability details

Impact

The Basket contract is intended to be used behind a proxy. But the ERC721 implementation used is not upgradeable, and its constructor is called at deployment time on the implementation. So all proxies will have a void name and symbol, breaking all potential integrations and listings.

Proof of Concept

ERC721("NFT Basket", "NFTB") is called at deployment time, and sets private variable at the implementation level. Therefore when loading the code during delegateCall, these variables will not be initialized.

Recommended Mitigation Steps

The easiest mitigation would be to pass this variable as immutable so they are hardcoded in the implementation byte code.

GalloDaSballo commented 2 years ago

Finding is valid, impact is the name of the tokens

HardlyDifficult commented 2 years ago

Confirmed this is an issue.

Assets are not at risk, and the function of the protocol is not impacted. All baskets created will have an empty name/symbol but generally there is no requirement that these values are populated. It's mostly for a better experience on frontends including etherscan. Downgrading and merging with the warden's QA report #314

Picodes commented 2 years ago

@HardlyDifficult Indeed it does not break the protocol's logic and funds are not at risk, but the name and the symbol of the NFTs are not the ones chosen by the sponsor, and as it's the core of EIP721Metadata we could argue that the function of the protocol are impacted. Also the experience on frontends (etherscan, opensea, etc) would have been significantly degraded. It could easily be considered a medium issue to me - especially considering the previous comments / reactions and the label "confirmed" added by the sponsor while it was high.

HardlyDifficult commented 2 years ago

Thanks @Picodes! I can get onboard with that line of thinking. Given how significant these fields are for 3rd party integrators such as Etherscan and Opensea this can be considered to fall under that definition of Medium risk. I'll upgrade this report and the dupes to Medium.