[PNM-005] Reentrancy of function `sell`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L381

Vulnerability details

Description

In function _chargeFee, a potential reetrancy can be triggered by the factory. Since function sell invokes _chargeFee in the middle of its function body, it potentially impacts the calculation, e.g., the totalSupply() would be incorrect since the tokens have not been burnt.

However, since factory is deployed by the development team, it may not be an issue (can also be low risk I believe).

Suggested Fix

Add reentrancy guard for sell just like buy. Or simply use transfer to transfer ETH.

[mundhrakeshav]

We do a sendEth tx at the end of function after all state updates. Isn't it good enough?

[GalloDaSballo]

The finding lacks a POC or Code detailing how the reEntrancy could be exploited

[HardlyDifficult]

We do a sendEth tx at the end of function after all state updates. Isn't it good enough?

The reentrancy here is via the send ETH within _chargeFee. So there is state changes that happen later, e.g. burn.

The finding lacks a POC or Code detailing how the reEntrancy could be exploited

Agree POCs are always helpful when submitting issues like this.

As the warden noted, the potential reentrancy must come from the fee recipient defined by admins inside the factory. Since this address is under admin control, it seems reasonable to not protect against reentrancy in order to keep gas costs down. Additionally if they did choose to reenter, the warden noted some state that could be relevant but has not detailed what harm could result. For these reasons I'm closing as invalid.