1

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

  • First thing's first, please make use of SafeMath to avoid integer overflows & underflows. – Jay Jan 9 at 13:24
  • It seems ok. (1) is there any other code in the function? (2) please show screenshot of remix error? Thanks – Yogesh - EtherAuthority.io Jan 9 at 13:35
  • What version of the Solidity compiler are you using? – Jay Jan 9 at 13:50
  • 1
    What is the reason for making this function payable? It doesn't look like you are handling an Ether transaction with this function. – Jay Jan 9 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 Jan 9 at 15:09
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.

  • 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 Jan 9 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 Jan 9 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 Jan 9 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 Jan 9 at 15:22
  • Simply swap the two code lines ;-) – Rick Park Jan 9 at 15:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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