code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/Tokens/Erc20.sol#L78-L84

Vulnerability details

Impact

А possible attack scenario:

Alice allows Bob to transfer N of Alice's tokens (N>0) by calling the approve method on a Token smart contract, \ passing the Bob's address and N as the method arguments
After some time, Alice decides to change from N to M (M>0) the number of Alice's tokens Bob is allowed to transfer, \ so she calls the approve method again, this time passing the Bob's address and M as the method arguments
Bob notices the Alice's second transaction before it was mined and quickly sends another transaction \ that calls the transferFrom method to transfer N Alice's tokens somewhere
If the Bob's transaction will be executed before the Alice's transaction, \ then Bob will successfully transfer N Alice's tokens and will gain an ability to transfer another M tokens
Before Alice noticed that something went wrong, Bob calls the transferFrom method again, this time to transfer M Alice's tokens.

So, an Alice's attempt to change the Bob's allowance from N to M (N>0 and M>0) made it possible for Bob to transfer N+M of Alice's tokens, \ while Alice never wanted to allow so many of her tokens to be transferred by Bob.

Proof of Concept

File: /Tokens/Erc20.sol

78    function approve(address spender, uint256 amount) public virtual returns (bool) {
79        allowance[msg.sender][spender] = amount;
80
81        emit Approval(msg.sender, spender, amount);
82
83        return true;
84    }

https://github.com/code-423n4/2022-07-swivel/blob/main/Tokens/Erc20.sol#L78-L84

Tools Used

editor

Recommended Mitigation Steps

Add decreaseAllowance and increaseAllowance functions on the custom ERC20 implementation.

JTraversa commented 2 years ago

Never been a fan of these reports, but i'll leave it to the judges. We just used the minimalistic solmate ERC20 implementation and in general the increase/decrease isnt a factor for our zcToken in comparison to approve.

bghughes commented 2 years ago

Never been a fan of these reports, but i'll leave it to the judges. We just used the minimalistic solmate ERC20 implementation and in general the increase/decrease isnt a factor for our zcToken in comparison to approve.

Agreed, this is QA at best IMO and the user is referencing an idea that applies to ERC20 globally

bghughes commented 2 years ago

Warden did not submit QA so this will act as their primary QA report.

robrobbins commented 2 years ago

may as well label this

"Use of ERC20"

code-423n4 / 2022-07-swivel-findings