Withdraw function in Astaria Router takes approval for wrong amount leading to possibility of over approvals

[code423n4]

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L48
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/AstariaRouter.sol#L168
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L131
https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/PublicVault.sol#L163

Vulnerability details

Impact

withdraw() function in Line 168 of AstariaRouter.sol allows vault token owners to withdraw underlying asset from vault. Note that implementation calls the withdraw() function of its parent contract ERC4626RouterBase.sol, refer line 48.

Note that approval is taken for amount instead of maxSharesOut - amount here refers to the actual underlying asset that was deposited in the vault (eg. WETH) and not the shares held by owner.

This function then calls withdraw function in Line 131 of PublicVault.sol. This function reduces allowance of shares that are supposed to be burnt - since the approval was given for amount of underlying asset & not maxShares, this could lead to either over allowance/under allowance situation. Under allowance leads to denial-of-service, over allowance leads to open approvals that were never supposed to be open.

Since this can potentially lead to loss of vault tokens of LPs, I've marked it as HIGH risk

Proof of Concept

• Bob is a LP who owns 100 shares in vault whose present vaule is 1000 ETH (let's assume each vault share is worth 10 ETH)
• Bob calls the withdraw function of Astaria Router
• Bob wants to withdraw 100 WETH by burning a maximum of 10 vault shares
• Function takes approval for 100 shares instead of 10
• Even after withdrawal, Bob still unknowingly has given approval to vault to spend 90 more shares representing 900 ETH

Tools Used

Manual

Recommended Mitigation Steps

Change line 48 in ERCRouterBase.sol from

ERC20(address(vault)).safeApprove(address(vault), amount);

to

ERC20(address(vault)).safeApprove(address(vault), maxSharesOut);

[c4-judge]

Picodes marked the issue as primary issue

[Picodes]

Medium risk to me as:

• Funds are not lost as the user can interact directly with the vault
• over allowance is not an issue in itself if there is no demonstrated attack path

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-sponsor]

SantiagoGregory marked the issue as sponsor confirmed

[androolloyd]

@SantiagoGregory

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked issue #228 as primary and marked this issue as a duplicate of 228