sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

SolidityATL - balancedVault's approve function is vulnerable to frontrunning attacks #169

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

SolidityATL

medium

balancedVault's approve function is vulnerable to frontrunning attacks

Summary

The approve function in balancedVault is vulnerable to frontrunning attacks

Vulnerability Detail

  1. Alice approves Bob of the ability to spend m amount of shares
  2. Alice updates Bob's allowance to n
  3. Bob frontruns Alice's allowance update and spends his m allocation
  4. Bob now has an allowance of n

In the following description Bob frontran the approval to have a total of m + n amount of shares.

Impact

The owner of shares can lose shares when approving a spender.

Code Snippet

https://github.com/equilibria-xyz/perennial-mono/blob/b06d5145db62a312dd88dfcafef0f8e2166c5e32/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L236-#L240

Tool used

Manual Review

Recommendation

There is a two step process that should be followed when implementing a safe approval mechanism. First you should only allow approval to be called when setting the initial allowance or when resetting the allowance to 0. Secondly, there should be two separate allowance functions for increasing and decreasing the allowance. Refer to the following for additional understanding of increase allowance and decrease allowance:

https://forum.openzeppelin.com/t/explain-the-practical-use-of-increaseallowance-and-decreaseallowance-functions-on-erc20/15103/4