2

I am developing a smart contract that integrates with the DAI stable coin. In the function below, you can see that I have followed the check effects interaction pattern, so how come I still get a prompt from the Remix compiler that I am potentially violating the check effects interactions pattern?

function depositParty (uint amount) public payable {
    require (daitoken.balanceOf(msg.sender) >= amount);
    accountBalance [msg.sender] += amount;
    daitoken.transferFrom(msg.sender, address(this), amount);

enter image description here

5
  • First thing's first, please make use of SafeMath to avoid integer overflows & underflows.
    – Jay Welsh
    Commented Jan 9, 2020 at 13:24
  • It seems ok. (1) is there any other code in the function? (2) please show screenshot of remix error? Thanks Commented Jan 9, 2020 at 13:35
  • What version of the Solidity compiler are you using?
    – Jay Welsh
    Commented Jan 9, 2020 at 13:50
  • 2
    What is the reason for making this function payable? It doesn't look like you are handling an Ether transaction with this function.
    – Jay Welsh
    Commented Jan 9, 2020 at 14:40
  • 1
    require (daitoken.balanceOf(msg.sender) >= amount) is redundant with transferFrom. I think that it might be that remix plugin doesn't understand that balanceOf is a view function.
    – Ismael
    Commented Jan 9, 2020 at 15:09

2 Answers 2

0

First of all are you sure to add amount to the msg.sender account BEFORE than he pays by the daitoken his augmented balance? If I understood well, is a reentrancy attack hook quite perfect... you have the effect BEFORE the payment.

A part from this, very probably remix is not able to understand the reference to the daitoken contract, in the sense that it cannot be assured that the referenced code do not make some which potentially violates that criteria.

6
  • Wouldn't the whole function call fail (and revert any state updates) if the balance wasn't sufficient due to SafeMath sub function throwing an error within the DAI contract if the amount is greater than the balance held (from causing an integer underflow)?
    – Jay Welsh
    Commented Jan 9, 2020 at 15:09
  • Yes, but if it is sufficient it give local balance before being paid and this is the problem. You should before be paid, then to add local balance.
    – Rick Park
    Commented Jan 9, 2020 at 15:11
  • Would the order still matter if the transferFrom was within a require()? (From a security perspective, not necessarily a best practice perspective, just curious)
    – Jay Welsh
    Commented Jan 9, 2020 at 15:15
  • Yes Jay: you should think that, under particular attacks, the attacker could “stop” the code from terminate the execution at a certain point of the listing. This is the reason why in a standard erc20 transfer you find the diminishing of the “from” account before the augmenting of the “to” account. Try to think that the attacker could be able to execute the code for a while and to avoid to finish the run in some way. You must be always sure that nobody can have some double spending like advantage in doing so. In your code any attacker able to execute just before the payment by DAI will do.
    – Rick Park
    Commented Jan 9, 2020 at 15:22
  • Simply swap the two code lines ;-)
    – Rick Park
    Commented Jan 9, 2020 at 15:23
0

You have checks interactions effects instead of checks effects interactions. Swap the latter two lines so that you first do the transfer and then you update the balance.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.