search

code-423n4 / 2022-02-jpyc-findings

1 stars 0 forks source link

QA report #10

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-jpyc/blob/cfc018384dd1d71febaa57f0576cb51f5d9c7e07/contracts/v1/FiatTokenV1.sol#L240-L249

Vulnerability details

Impact

When user A call approve to change allowance of user B, user B can front run and use all the user A given allowance.

Proof of Concept

Exploitation scenario

  1. Alice calls approve(Bob, 1000), allocating 1000 tokens for Bob to spend
  2. Alice opts to change the amount approved for Bob to spend to a lesser amount via approve(Bob, 500). Once mined, this decreases the number of tokens that Bob can spend to 500.
  3. Bob sees the transaction and calls transferFrom(Alice, X, 1000) before approve(Bob, 500) has been mined.
  4. If Bob’s transaction is mined prior to Alice’s, 1000 tokens will be transferred by Bob.
  5. However, once Alice’s transaction is mined, Bob can call transferFrom(Alice, X, 500), transferring a total of 1500 tokens despite Alice attempting to limit the total token allowance to 500.

Recommended Mitigation Steps

Add the check require(allowed[msg.sender][_spender] == 0)

thurendous commented 2 years ago

We don't think this is an issue at the moment. We forked centre-token and follow the ERC20 standard, and there isn't such problem being mentioned.

thurendous commented 2 years ago

Base on that people should pay attention by himself for how they approve their tokens and it is following the standards of centre-token and ERC20, we argue that this is a low-risk issue.

CloudEllie commented 2 years ago

Since this issue was downgraded to a QA level, and the warden did not submit a separate QA report, we've renamed this one to "QA report" for consistency.

The original title, for the record, was "Race condition ERC20 approve."