Transfer amount calculated incorrectly in claim() function, could underflow balance

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-astaria/blob/1bfc58b42109b839528ab1c21dc9803d663df898/src/WithdrawProxy.sol#L268-L282

Vulnerability details

Impact

When claim() is called on the WithdrawProxy, the goal is to allow the vault to claim the share of the balance that is not a part of the withdrawRatio.

withdrawRatio is calculated as a share of 1e18, so the ideal math is as follows:

transferAmount = balance * withdrawRatio / 1e18;
sendToVault = balance * (1 - (withdrawRatio / 1e18));

Instead, the function uses 10**ERC20(asset()).decimals() in place of 1e18. This is equivalent when working with a token with 18 decimal places, but for tokens with different numbers of decimals, this can dramatically throw off the expected math.

Proof of Concept

Let's say withdrawRatio == 1e17 (ie 10%) and we are using a token with 8 decimal places.

transferAmount is calculated as: transferAmount = uint256(s.withdrawRatio).mulDivDown( balance, 10**ERC20(asset()).decimals() );

This simplifies to 1e17 * balance / 1e8 or 1e9 * balance.

This is a major problem, because transferAmount is supposed to be a share of balance, and should never be greater than balance. We can see this in the next line, when an unchecked block is used to subtract the two:

unchecked {
    balance -= transferAmount;
}

The result is that, if we use a token with fewer decimals than 18, balance will underflow.

Tools Used

Manual Review

Recommended Mitigation Steps

Replace 10**ERC20(asset()).decimals() with 1e18 in the transferAmount calculation.

[c4-judge]

Picodes marked the issue as duplicate of #482

[c4-judge]

Picodes marked the issue as satisfactory