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

Support Contracts
1

Hi there! First post here and very new Solidity dev (2 weeks now) so sorry if this is an obvious question.

I’m creating an ERC-721 contract and using a few OpenZeppelin contracts as parent contracts: Ownable, ERC721, and PullPayment.

Generally what I want to do is mint a token, then send a percentage of that payment to some other addresses.

I’ve been reading up on best practices and since I will be disbursing funds to addresses programmatically from my contract, I want to ensure it is as secure as possible.

Some pseudocode for context, followed by questions.

contract MyNFT is Ownable, ERC721, PullPayment {

Counters.Counter private nftId;
...

    function mintAToken() external payable {
        // CHECKS - are calls to parent contracts like ERC721 ok?
        // ensure the max supply has not been reached
        require(totalSupply() < maximumTokens, "No supply");
        // maximum one piece per address
        require(balanceOf(msg.sender) == 0, "One per address");

        // EFFECTS + INTERACTIONS (unclear)

        // mint the token
        _mint(msg.sender, nftId);
        nftId.increment();

        // emit events - does this count as effect or interaction?
        emit NewToken(some info about token);

        // send money to charity or someone else
        // depositToEscrow is wrapper around PullPayment's _asyncTransfer
        // external payable because apparently you can't do internal payable
        uint256 percentOfPayment = getPercentOfPayment(msg.value, somePercent);
        depositToEscrow(beneficiary, percentOfPayment);
    }

}

  1. 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?

  2. 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?

  3. 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)

  4. 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)?

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

Thanks in advance, and also thanks so much to the Open Zeppelin team for your amazing contracts, it’s made everything way easier.

1 Like
2

Hi @tychecollective,

Welcome to the community :wave:

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_

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

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

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

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

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.

3

Hi! Thank you for your excellent answers all over this forum.

I have a question about this answer please.

What constitutes an untrusted contract?

For instance, if as parts of Checks you call an external, non-parent contract, whose address you know and set explicitly, would this be untrusted or trusted?

pseudo code example

contract CheckOwner {

address constant knownERC721Contract;
uint256 constant tokenID;

//Checks
require( msg.sender == knownERC721Contract.ownerOf(tokenID), "you do not own the token");

//...
}

Thanks!