search

code-423n4 / 2021-05-visorfinance-findings

0 stars 0 forks source link

Double-spend allowance is possible in the function `approveTransferERC20` #79

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

shw

Vulnerability details

Impact

This issue is similar to the well-known double-spend problem in the ERC20 approve function. A user may transfer out more tokens than the owner approved by front-running the transactions, which set the user's allowance to a different value.

Proof of Concept

  1. The owner calls approveTransferERC20(DAI, Alice, 10) to approve Alice to transfer 10 DAI from the vault.
  2. After some time, the owner decides to change the allowance to 5, so he calls approveTransferERC20(DAI, Alice, 5).
  3. Alice, who keeps monitoring the transactions in the mempool, notices the transaction sent in Step 2. She then front-runs that transaction (by either a higher gas price or using flashbot bundles) and calls delegatedTransferERC20(DAI, Alice, 10) to transfer 10 DAI out of the vault.
  4. The transaction sent in Step 2 successfully executes and sets the allowance of Alice to 5.
  5. Alice then transfers out 5 more DAI by calling delegatedTransferERC20 again. As a result, she could transfer more tokens from the vault than the owner intended.

Referenced code:

Visor.sol#L430-L463

Tools Used

None

Recommended Mitigation Steps

Implement the increaseAllowance and decreaseAllowance functions instead, as suggested by OpenZeppelin's ERC20 implementation.

ghost commented 3 years ago

sponsor acknowledged disagree with severity 1 This issue with ERC20 approve pattern is noted. We will switch to an increase,decrease approval in next vault update. (experimental feature)

ghoul-sol commented 3 years ago

Duplicate of #10