Double allowance spending when using the approve() function.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/usdy/rUSDY.sol#L276-L279

Vulnerability details

Impact

• Malicious spenders could double spend from the owners if the owner decreases their allowance using the approve() function

Proof of Concept

• If an owner wants to set the allowance of an spender to a new value (either remove or add more) and uses the rUSDY:allowance() function, it needs to send the value of the amount parameter set to the exact new allowance that the owner grants to the spender.

• Let's take as example: UserA (owner) granted 1000 allowance to UserB (spender), and after some time, the owner decides to decrease the total allowance of UserB down to 250.

  • The spender realizes about the tx of the owner to decrease its allowance and frontruns that tx by sending a tx to spend all the existing allowance, so it transfersFrom the owner the currentAllowance, which is 1000.

  • After the spender tx was executed, the tx of the owner to decrease the spender's allowance is executed, and now the spender has been granted with another 250 extra allowance.

  • Now the spender goes ahead and sends another tx to spend the just recent increased allowance, and spends another 250 tokens from the owner.

    • The result is that the spender effectively spent 1250 tokens from the owner, when the owner only wanted to allow the spender to spend 250 tokens.

function approve(address _spender, uint256 _amount) public returns (bool) {
  //@audit-info => It will set the spender's allowance to the value of _amount!
  _approve(msg.sender, _spender, _amount);
  return true;
}

Tools Used

Manual Audit

Recommended Mitigation Steps

• Deprecate the approve(), remove it from the contract. Leave only the increaseAllowance() and decreaseAllowance() functions.

Assessed type

ERC20

[raymondfam]

Caution users to use increaseAllowance() then.

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

kirk-baird marked the issue as unsatisfactory:
Invalid

[stalinMacias]

@kirk-baird
Can I ask what is missing on my report for it to be marked as a low-quality report?

I considered the explanation provided in the PoC is self-explanatory about the issue I'm reporting, but, pls, let me know what do you think it was missing in the report

[kirk-baird]

@kirk-baird Can I ask what is missing on my report for it to be marked as a low-quality report?

I considered the explanation provided in the PoC is self-explanatory about the issue I'm reporting, but, pls, let me know what do you think it was missing in the report

Hi @stalinMacias this issue was not marked as low quality due to the writing style or PoC. I found these sufficient quality. It's marked as low-quality due to the issue itself.

Double spending on approve() is a known issue for ERC20 to which the solution is to add the functions increaseAllowance() and decreaseAllowance(). Since they've already added those functions this issue is already resolved.

[stalinMacias]

@kirk-baird Thanks for the explanation, will take this into account for future submissions