Secure design (checks-effects-interactions) & parent/child contracts: what counts as calling external contract?

[abcoathup]

Hi @tychecollective,

Welcome to the community

If you haven't already I suggest looking at: Reentrancy After Istanbul.

I suggest only including functionality in a smart contract that is required for the life of the smart contract. If you have sale functionality for a token, then assuming the sale isn't for the life of the token, then you may want to consider adding the sale functionality to a contract dedicated to the sale and leave the token as simple as possible. ERC20 crowdsales are an example of this: https://docs.openzeppelin.com/contracts/3.x/crowdsales

I am definitely not an expert on reentrancy, I am a Community Manager and not a Security Researcher.

If at any point of execution we are unsure whether our contract’s invariants hold or not, we should avoid calling other (untrusted) contracts, because they could re-enter. If we have no choice but to do so, we can try to prevent reentrancy by using a reentrancy guard.
_From: https://forum.openzeppelin.com/t/reentrancy-after-istanbul/1742_

@tychecollective

Is calling functions derived from parent contracts (i.e. owner() from Ownable or totalSupply() from ERC721 ) ever seen as an ‘external’ call that could be used for reentrancy/other vulnerabilities?

I assume these would generally be checks (no state change) or effects (state change). Though a parent contract could potentially call an external contract.

@tychecollective

Additionally in the example above, is calling _mint from ERC721 an interaction and therefore no state changing should be done after it? Is incrementing the nftId counter after minting a vulnerability?

_mint is an effect (state change), so is the increment and are both internal to your contract. You have grouped your effects together.

@tychecollective

Is calling external functions in the require statements (checks) at the beginning of a function a vulnerability? (including something like totalSupply() from the parent contracts, though that may be answered in 1)

If you are calling an untrusted contract, then yes. If you are calling the parent then this is internal and should generally be ok.

@tychecollective

Is calling something from PullPayments seen as external since it ends up calling the Escrow contract created by my contract MyNFT (and ditto for any other child contracts of my contract)?

These should be fine, again, you need to be concerned about calling an untrusted contract.

@tychecollective

Lastly, a general question: what is emitting events seen as? Can I do it safely after interactions with external contracts?

I am not sure what they are classified as, but you are emitting an event which can't be used to call an external contract.

---

As a general note, contracts with value should be appropriately tested and audited.