Race condition in the `Comp.approve` approve function may lead to token theft

[aviggiano]

Affected smart contract

https://github.com/LodestarFinance/lodestar-protocol/blob/relaunch-candidate/contracts/Governance/Comp.sol#L86-L98

Description

The ERC20 standard contains a known race condition on the approve function, making possible the theft of tokens. The ERC20 standard describes how to create generic token contracts. Among others, a ERC20 contract has to define these two functions:

• transferFrom(from, to, value)
• approve(spender, value)

The goal of these functions is to give the permission to a third party to spend tokens. Once the function approve(spender, value) has been called by a user, spender can spend up to value tokens of the user by calling transferFrom(user, to, value).

This schema is vulnerable to a race condition when the user calls approve a second time on an already allowed spender. If the spender sees the transaction containing the call before it has been mined, they can call transferFrom to transfer the previous value and still receive the authorization to transfer the new value.

Reference: https://www.trailofbits.com/documents/sai.pdf

Exploit Scenario

1. Alice calls approve(Bob, 500). This allows Bob to spend 500 tokens.
2. Alice changes her mind and calls approve(Bob, 1000). This changes the number of tokens that Bob can spend to 1000.
3. Bob sees the transaction and calls transferFrom(Alice, X, 500) before it has been mined.
4. If Bob’s transaction is mined before Alice’s, 500 tokens will be transferred by Bob. Once Alice’s transaction is mined, Bob can call transferFrom(Alice, X, 1000).

Bob has transferred 1500 tokens even though this was not Alice’s intention.

Recommendation

One possible solution is to forbid a call to approve if all the previous tokens are not spent by adding a require to approve. This solution prevents the race condition but it may result in unexpected behavior for a third party.

require(allowances[msg.sender][spender] == 0);

Alternatively, use OpenZeppelin's ERC20 increaseAllowance and decreaseAllowance functions to mitigate this issue.

[0xAppo]

This is a known issue in the ERC-20 standard.