search

code-423n4 / 2023-07-moonwell-findings

1 stars 0 forks source link

Race condition in approve function can lead to more funds than intended being transferred #277

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/main/src/core/MToken.sol#L159-L164

Vulnerability details

Impact

The approve() function from MToken.sol contains a front-running vulnerability that allows a user to spend more tokens than he should.

Proof of Concept

Lets take the following scenario:

  1. Alice calls approve(Eve, 10). This permits Eve to spend 10 tokens from Alice.
  2. Alice changes her mind and calls approve(Eve, 5) to only allow Eve to spend 5 of her tokens, instead of 10.
  3. Eve sees Alice's transaction and sends her own transaction calling transferFrom(Alice, Eve, 10), submitting it with a higher gas price than Alice.
  4. If Eve's transaction gets included in the block before Alice's is, 10 tokens from Alice will be transferred to Eve. Then, after Alice's transaction is included, Eve can call transferFrom again, this time with the newly approved 5 tokens.
  5. As a consequence, Alice only wanted and expected Eve to spend 5 of her tokens, while in reality, because of the race condition vulnerability, Eve was able to take 15 of Alice's tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

One solution is to forbid a call to approve if the previous approved tokens were not spent. This would prevent the race condition: require(allowances[Alice][Eve] == 0, "error");

Another solution is to add 2 function to increase and decrease the allowances. Check the OpenZeppelin ERC20 implementation: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.9.3/contracts/token/ERC20/ERC20.sol

Assessed type

ERC20

0xSorryNotSorry commented 1 year ago

The race condition is a feature of approve(), however, it doesn't harm the protocol or the end user as the approve was voluntarily given.

Could be QA.

c4-pre-sort commented 1 year ago

0xSorryNotSorry marked the issue as low quality report

alcueca commented 1 year ago

No front-running approve vulnerabilities in my court!

c4-judge commented 1 year ago

alcueca marked the issue as unsatisfactory: Insufficient quality