`approve()` can be front-ran resulting in token theft.

[code423n4]

Lines of code

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CToken.sol#L156

Vulnerability details

Impact

In CToken.sol#L156, the ERC20's approve() function has a known race condition that can lead to token theft. If a user calls the approve function a second time on a spender that was already allowed, the spender can front-run the transaction and call transferFrom() to transfer the previous value and still receive the authorization to transfer the new value.

Proof of Concept

CToken.sol#L156.

Exploit Scenario:

1. Alice allows Bob to transfer 20 of Alice's tokens by calling approve method on Token smart contract passing Bob's address and 20 as method arguments. approve("0xBob", 20);
2. After some time, Alice decides to change from 20 to 10 the number of Alice's tokens Bob is allowed to transfer, so she calls approve method again, this time passing Bob's address and 10 as method arguments. approve("0xBob", 10);
3. Bob notices Alice's second transaction before it was mined and quickly sends another transaction that calls transferFrom method to transfer 20 Alice's tokens to himself. transferFrom("0xAlice", "0xBob", 20);
4. If Bob's transaction will be executed before Alice's transaction, Bob will successfully transfer 20 Alice's tokens and gain the ability to transfer another 10 tokens.
5. Before Alice noticed that something went wrong, Bob calls transferFrom method again, this time to transfer 10 Alice's tokens. This results in Bob acquiring 30 Alice's tokens instead of only 10.

   Tools Used

   VS Code.

   Recommended Mitigation Steps

   Use safeIncreaseAllowance() and safeDecreaseAllowance() from OpenZeppelin’s SafeERC20 implementation to prevent race conditions from manipulating the allowance amounts

[bunkerfinance-dev]

Duplicate of #50

[gzeoneth]

Downgrading to Low / QA.

[gzeoneth]

Consider with warden's QA report #90