1

Please take the following hypothetical code as an example

mapping (address => uint256) public accountBalance;

function deposit(uint256 amount) public {
    accountBalance[msg.sender] = accountBalance[msg.sender].add(amount);
    require(daiToken.transferFrom(msg.sender, address(this), amount));
}

I have been informed that this code would be vulnerable to a re-entrancy attack and should instead be written as follows:

mapping (address => uint256) public accountBalance;

function deposit(uint256 amount) public {
    require(daiToken.transferFrom(msg.sender, address(this), amount));
    accountBalance[msg.sender] = accountBalance[msg.sender].add(amount);
}

How is it possible to cause a state update to the local contract if there is a require statement wrapped around the transferFrom function?

Kindest Regards

2

If daiToken were a malicious contract once you have invoked transferFrom then it can call your deposit function before it has finished executing the first time so potentially amount is credited multiple times.

But to be honest ERC20 doesn't guarantee even if you call transferFrom successfully that the transfer of tokens will be OK. You have to trust that it will behave correctly.

|improve this answer|||||
  • Thanks, could you possibly elaborate on how an ERC20 token's transferFrom function could return true without actually being a successful transfer (assuming an ERC20 implementation such as OpenZepplin's)? – Jay Jan 9 at 18:33
  • I do still wonder, though, even if daiToken was malicious, and if it called back into the deposit function from the daiToken contract, how would the accountBalance ever be incorrectly incremented if the "require(daiToken.transferFrom(...))" in the following line fails? Or is the idea that the danger here is simply that the deposit function ends up depositing more user funds than initially intended? – Jay Jan 9 at 18:40
  • @Jay OpenZeppelin's implementation obviously do the right thing (tm). An arbitrary implementation can just fire a Transfer event and return true, it is a valid ERC20 token because it follows the specification. In general when a contracts calls another, for example at daiToken.transferFrom() it is passing the control flow to daiToken and that contract can do anything. – Ismael Jan 9 at 19:48
  • @Jay The danger is that your contract will be in an temporary inconsistent state, user balance in your contract has increased but tokens aren't in your contract yet. So temporarily the user has its own tokens, and your contract think it has deposited the tokens there. By itself this is not enough to exploit the contract but it is a ticking bomb, together with another failure it might be exploitable. – Ismael Jan 9 at 19:54
  • 1
    @Jay Honestly I don't know if there's a good answer. Solidity allows to write unsafe code very easily. Swapping two lines in your example will fix it, but it is hard to recognize that type of errors even for experienced developers. There some research about using formal frameworks to proof that a contract is secure by design, but they are advances in academia not ready for everyone to use them. – Ismael Jan 10 at 0:18
1

There is reentrancy example code you can try out here: https://github.com/KevinSmall/SolidityContractVulnerabilities#reentrancy

|improve this answer|||||
  • Thanks, that is fantastic! – Jay Jan 10 at 14:11

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.