Improper State Re-Entrancy

[aaitor]

TNT-01M: Improper State Re-Entrancy

Type	Severity	Location
Logical Fault	Medium	TransferNFT721Condition.sol:L137

Description:

The fulfill function performs a safeTransferFrom invocation on an ERC-721 token which notifies the recipient of the transfer. In case the recipient of the transfer is a smart contract, the recipient will be able to re-enter the contract and potentially set the same ID as fulfilled twice and emit a corresponding event as well as generally cause an incorrect state transition of the system.

Example:

if (_nftAmount == 1) {
    token.safeTransferFrom(msg.sender, _nftReceiver, uint256(_did));
}

ConditionStoreLibrary.ConditionState state = super.fulfill(
    _id,
    ConditionStoreLibrary.ConditionState.Fulfilled
);

emit Fulfilled(
    _agreementId,
    _did,
    _nftReceiver,
    _nftAmount,
    _id,
    _contract
);

return state;

Recommendation:

We advise re-entrancy to be prohibited here by either introducing the nonReentrant modifier of OpenZeppelin throughout the codebase, simply performing a transferFrom instead of a safeTransferFrom as both transfers are safely done despite what their name implies, or re-order the statements to perform the transfer at the bottom of the function. We should note that if transferFrom is used, the recipient is expected to be ERC-721 aware as otherwise the tokens transmitted could be permanently lost.