search

code-423n4 / 2021-09-yaxis-findings

0 stars 0 forks source link

`Vault.balanceOfThis` values all tokens equally #120

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

cmichel

Vulnerability details

The Vault.balanceOfThis function values all tokens equally. They are normalized to 18 decimals and then simply added up:

for (uint8 i; i < _tokens.length; i++) {
    address _token = _tokens[i];
    // adds up different tokens here, treating them as exactly equal value 1.0 A = 1.0 B
    _balance = _balance.add(_normalizeDecimals(_token, IERC20(_token).balanceOf(address(this))));
}

Impact

The balanceOfThis function does not take into account the actual market prices these tokens trade at. It would treat 1.0 USDC as 1.0 WETH and simply add these values. Even if only stablecoins are used, they can still differ in value, especially with non-reserve-backed algorithmic stablecoins like DAI.

If DAI is valued at 0.99$ but USDC is valued at 1.00$, then arbitragers will deposit(DAI, amount), receive shares, withdraw the shares for USDC (withdraw(shares, USDC)) and make a profit (assuming the _withdrawalProtectionFee is set to an adequate value). The contract will run out of the higher-valued USDC asset immediately, and overall lose total value.

Recommended Mitigation Steps

Use a price oracle for each token with a common denominating asset, iterate over the tokens, convert them with the oracle price, and then add them up.

Alternatively, set a high withdrawal fee _withdrawalProtectionFee but this is bad for the user and could turn off users from depositing anything. Assume one gets 12% APY but the withdrawal fee is set to 2%, a user will lose money if they withdraw before 2 months.

Haz077 commented 2 years ago

Duplicate of #2.

GalloDaSballo commented 2 years ago

Duplicate of #2