code-423n4 / 2023-01-astaria-findings

5 stars 2 forks source link

Upgraded Q -> 2 from #596 [1677228840417] #634

Closed c4-judge closed 1 year ago

c4-judge commented 1 year ago

Judge has assessed an item in Issue #596 as 2 risk. The relevant finding follows:

withdraw and redeem https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L41-L52

function withdraw( IERC4626 vault, address to, uint256 amount, uint256 maxSharesOut ) public payable virtual override returns (uint256 sharesOut) {

ERC20(address(vault)).safeApprove(address(vault), amount); if ((sharesOut = vault.withdraw(amount, to, msg.sender)) > maxSharesOut) { revert MaxSharesError(); } } https://github.com/AstariaXYZ/astaria-gpl/blob/4b49fe993d9b807fe68b3421ee7f2fe91267c9ef/src/ERC4626RouterBase.sol#L55-L66

function redeem( IERC4626 vault, address to, uint256 shares, uint256 minAmountOut ) public payable virtual override returns (uint256 amountOut) {

ERC20(address(vault)).safeApprove(address(vault), shares); if ((amountOut = vault.redeem(shares, to, msg.sender)) < minAmountOut) { revert MinAmountError(); } } Both function calls to ERC20.approve are not valid, since ERC4626.withdraw will burn the shares and no transfers of shares are needed.

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #175

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory