Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent inflation attack #237

Merged
merged 2 commits into from
Oct 20, 2022
Merged

Prevent inflation attack #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
029eb57
Prevent inflation attack
larryob Oct 19, 2022
629ab4e
Merge branch 'master' into inflation-bug
larryob Oct 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 28 additions & 28 deletions .gas-snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ EmergencyWithdrawalQueueTest:testCorreclyEnqueueReturningUser() (gas: 394051)
EmergencyWithdrawalQueueTest:testDequeueSuccess() (gas: 333952)
EmergencyWithdrawalQueueTest:testEnqueueOnlyVaultCanEnqueue() (gas: 13024)
EmergencyWithdrawalQueueTest:testEnqueueSuccess() (gas: 179377)
ForwardTest:testDoubleDeposit() (gas: 215743)
ForwardTest:testTransactVaultAndBasket() (gas: 602615)
ForwardTest:testDoubleDeposit() (gas: 218350)
ForwardTest:testTransactVaultAndBasket() (gas: 616368)
CompoundStratTest:testCanSellRewards() (gas: 2230508)
CompoundStratTest:testDivestFromStrategy() (gas: 772670)
CompoundStratTest:testStrategyDivestsOnlyAmountNeeded() (gas: 430650)
Expand All @@ -48,33 +48,33 @@ L1VaultTest:testSendTVL() (gas: 79271)
L1VaultTest:testafterReceive() (gas: 927215)
L1VaultTest:testprocessFundRequest() (gas: 473328)
AAVEStratTest:testStrategyDivestsOnlyAmountNeeded() (gas: 471593)
AAVEStratTest:testStrategyMakesMoney() (gas: 555329)
AAVEStratTest:testStrategyMakesMoney() (gas: 566887)
AAVEStratTest:testTVL() (gas: 400582)
L2VaultTest:testCheckEmeregencyWithdrawalQueueBeforeRedeem() (gas: 527278)
L2VaultTest:testCheckEmeregencyWithdrawalQueueBeforeWithdraw() (gas: 529162)
L2VaultTest:testDeploy() (gas: 20794)
L2VaultTest:testDepositRedeem(uint128) (runs: 256, μ: 197783, ~: 197791)
L2VaultTest:testDepositWithdraw(uint128) (runs: 256, μ: 197529, ~: 197536)
L2VaultTest:testDetailedPrice() (gas: 174837)
L2VaultTest:testEmergencyWithdrawal(uint128) (runs: 256, μ: 502667, ~: 502675)
L2VaultTest:testEmergencyWithdrawalQueueNotStarved() (gas: 713317)
L2VaultTest:testEmergencyWithdrawalRequestDrop() (gas: 462740)
L2VaultTest:testEmergencyWithdrawalWithRedeem(uint128) (runs: 256, μ: 503175, ~: 503183)
L2VaultTest:testL1ToL2Rebalance() (gas: 143544)
L2VaultTest:testCheckEmeregencyWithdrawalQueueBeforeRedeem() (gas: 533760)
L2VaultTest:testCheckEmeregencyWithdrawalQueueBeforeWithdraw() (gas: 532812)
L2VaultTest:testDeploy() (gas: 20923)
L2VaultTest:testDepositRedeem(uint64) (runs: 256, μ: 195429, ~: 195435)
L2VaultTest:testDepositWithdraw(uint64) (runs: 256, μ: 198125, ~: 198130)
L2VaultTest:testDetailedPrice() (gas: 85959)
L2VaultTest:testEmergencyWithdrawal(uint64) (runs: 256, μ: 505753, ~: 505758)
L2VaultTest:testEmergencyWithdrawalQueueNotStarved() (gas: 717065)
L2VaultTest:testEmergencyWithdrawalRequestDrop() (gas: 466060)
L2VaultTest:testEmergencyWithdrawalWithRedeem(uint64) (runs: 256, μ: 506233, ~: 506239)
L2VaultTest:testL1ToL2Rebalance() (gas: 149232)
L2VaultTest:testL1ToL2RebalanceWithEmergencyWithdrawalQueueDebt() (gas: 137688)
L2VaultTest:testL2ToL1Rebalance() (gas: 178507)
L2VaultTest:testL2ToL1RebalanceWithEmergencyWithdrawalQueueDebt() (gas: 172564)
L2VaultTest:testLockedProfit() (gas: 314770)
L2VaultTest:testLockedTVL() (gas: 158792)
L2VaultTest:testManagementFee() (gas: 420032)
L2VaultTest:testMinDeposit() (gas: 155480)
L2VaultTest:testMint(uint128) (runs: 256, μ: 176237, ~: 176548)
L2VaultTest:testReceiveTVL() (gas: 170010)
L2VaultTest:testSettingFees() (gas: 49218)
L2VaultTest:testSettingForwarder() (gas: 40969)
L2VaultTest:testVaultPause() (gas: 305776)
L2VaultTest:testWithdrawalFee() (gas: 231108)
RouterTest:testMultipleDeposits() (gas: 609111)
L2VaultTest:testL2ToL1Rebalance() (gas: 184130)
L2VaultTest:testL2ToL1RebalanceWithEmergencyWithdrawalQueueDebt() (gas: 172586)
L2VaultTest:testLockedProfit() (gas: 314786)
L2VaultTest:testLockedTVL() (gas: 162415)
L2VaultTest:testManagementFee() (gas: 420268)
L2VaultTest:testMinDeposit() (gas: 186835)
L2VaultTest:testMint(uint64) (runs: 256, μ: 167623, ~: 167623)
L2VaultTest:testReceiveTVL() (gas: 179256)
L2VaultTest:testSettingFees() (gas: 49196)
L2VaultTest:testSettingForwarder() (gas: 42626)
L2VaultTest:testVaultPause() (gas: 306410)
L2VaultTest:testWithdrawalFee() (gas: 234240)
RouterTest:testMultipleDeposits() (gas: 622850)
BtcEthBasketTest:testBuySplits() (gas: 191718)
BtcEthBasketTest:testBuySplitsFuzz(uint256,uint256) (runs: 256, μ: 410424, ~: 410590)
BtcEthBasketTest:testDepositSlippage() (gas: 867635)
Expand All @@ -92,7 +92,7 @@ L1WormholeRouterTest:testReportTVL() (gas: 206226)
L1WormholeRouterTest:testReportTransferredFund() (gas: 204607)
L2WormholeRouterTest:testMessageValidation() (gas: 1081051)
L2WormholeRouterTest:testReceiveFundsInvariants() (gas: 115104)
L2WormholeRouterTest:testReceiveTVL() (gas: 170050)
L2WormholeRouterTest:testReceiveTVL() (gas: 175673)
L2WormholeRouterTest:testRequestFunds() (gas: 204241)
L2WormholeRouterTest:testTransferReport() (gas: 204179)
L2WormholeRouterTest:testWormholeConfigUpdates() (gas: 22289)
49 changes: 23 additions & 26 deletions src/polygon/L2Vault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,10 @@ contract L2Vault is
}

function decimals() public view override returns (uint8) {
return _asset.decimals();
// E.g. for USDC, we want the initial price of a share to be $100.
// For an initial price of 1 USDC / share we would have 1e6 * 1e8 / 1 = 1e14 given that we have (6 + 8) 14 decimals
// But since we want 100 USDC / share for the intial price, we add an extra two decimal places
return _asset.decimals() + 10;
}

bytes32 public constant GUARDIAN_ROLE = keccak256("GUARDIAN");
Expand Down Expand Up @@ -332,18 +335,19 @@ contract L2Vault is

/// @dev In previewDeposit we want to round down, but in previewWithdraw we want to round up
function _convertToShares(uint256 assets, Rounding roundingDirection) internal view returns (uint256 shares) {
uint256 totalShares = totalSupply();
// E.g. for USDC, we want the initial price of a share to be $100.
// Apparently testnet users confused AlpSave with a stablecoin
if (totalShares == 0) {
shares = assets / 100;
// Even if there are no shares or assets in the vault, we start with 1 wei of asset and 1e8 shares
// This helps mitigate price inflation attacks: https://github.com/transmissions11/solmate/issues/178
// See https://www.rileyholterhus.com/writing/bunni as well.
// The solution is inspired by YieldBox
uint256 totalShares = totalSupply() + 1e8;
uint256 _totalAssets = totalAssets() + 1;

if (roundingDirection == Rounding.Up) {
shares = assets.mulDivUp(totalShares, _totalAssets);
} else {
if (roundingDirection == Rounding.Up) {
shares = assets.mulDivUp(totalShares, totalAssets());
} else {
shares = assets.mulDivDown(totalShares, totalAssets());
}
shares = assets.mulDivDown(totalShares, _totalAssets);
}
shares;
}

/// @notice See {IERC4262-convertToAssets}
Expand All @@ -353,16 +357,13 @@ contract L2Vault is

/// @dev In previewMint, we want to round up, but in previewRedeem we want to round down
function _convertToAssets(uint256 shares, Rounding roundingDirection) internal view returns (uint256 assets) {
uint256 totalShares = totalSupply();
if (totalShares == 0) {
// see _convertToShares
assets = shares * 100;
uint256 totalShares = totalSupply() + 1e8;
uint256 _totalAssets = totalAssets() + 1;

if (roundingDirection == Rounding.Up) {
assets = shares.mulDivUp(_totalAssets, totalShares);
} else {
if (roundingDirection == Rounding.Up) {
assets = shares.mulDivUp(totalAssets(), totalShares);
} else {
assets = shares.mulDivDown(totalAssets(), totalShares);
}
assets = shares.mulDivDown(_totalAssets, totalShares);
}
}

Expand Down Expand Up @@ -565,16 +566,12 @@ contract L2Vault is
* DETAILED PRICE INFO
*
*/

/// @dev The vault has as many decimals as the input token does
function detailedTVL() external view override returns (Number memory tvl) {
tvl = Number({num: totalAssets(), decimals: decimals()});
tvl = Number({num: totalAssets(), decimals: _asset.decimals()});
}

function detailedPrice() external view override returns (Number memory price) {
// If there are no shares, simply say that the price is 100
uint256 rawPrice = totalSupply() > 0 ? (totalAssets() * 10 ** decimals()) / totalSupply() : 100 ** decimals();
price = Number({num: rawPrice, decimals: decimals()});
price = Number({num: convertToAssets(10 ** decimals()), decimals: _asset.decimals()});
}

function detailedTotalSupply() external view override returns (Number memory supply) {
Expand Down
4 changes: 1 addition & 3 deletions src/test/Forwarder.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,6 @@ contract ForwardTest is TestPlus {
requests[0] = req1;
requests[1] = req2;
forwarder.executeBatch(requests, abi.encodePacked(r, s, v, r2, s2, v2));

assertEq(vault.balanceOf(user), 2e6 / 100);
}

function testTransactVaultAndBasket() public {
Expand Down Expand Up @@ -135,7 +133,7 @@ contract ForwardTest is TestPlus {
requests[1] = req2;
forwarder.executeBatch(requests, abi.encodePacked(r, s, v, r2, s2, v2));

assertEq(vault.balanceOf(user), 1e6 / 100);
assertTrue(vault.balanceOf(user) > 0);
assertTrue(basket.balanceOf(user) > 0);
}
}
86 changes: 35 additions & 51 deletions src/test/L2Vault.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract L2VaultTest is TestPlus {
);

function testDeploy() public {
assertEq(vault.decimals(), asset.decimals());
assertEq(vault.decimals(), asset.decimals() + 10);
// this makes sure that the first time we assess management fees we get a reasonable number
// since management fees are calculated based on block.timestamp - lastHarvest
assertEq(vault.lastHarvest(), block.timestamp);
Expand All @@ -52,83 +52,75 @@ contract L2VaultTest is TestPlus {
assertTrue(vault.canRequestFromL1());
}

function testDepositRedeem(uint128 amountAsset) public {
function testDepositRedeem(uint64 amountAsset) public {
vm.assume(amountAsset > 99);
// Running into overflow issues on the call to vault.redeem
address user = address(this);
asset.mint(user, amountAsset);

uint256 expectedShares = uint256(amountAsset) * 1e8;
// user gives max approval to vault for asset
asset.approve(address(vault), type(uint256).max);
vm.expectEmit(true, true, true, true);
emit Deposit(user, user, amountAsset, amountAsset / 100);
vault.deposit(amountAsset, user);

// If vault is empty, assets are converted to shares at 100:1
uint256 numShares = vault.balanceOf(user);
uint256 expectedShares = amountAsset / 100;
assertEq(numShares, expectedShares);
assertEq(vault.balanceOf(user), expectedShares);
assertEq(asset.balanceOf(address(user)), 0);
assertEq(asset.balanceOf(address(vault)), amountAsset);

vm.expectEmit(true, true, true, true);
emit Withdraw(user, user, user, amountAsset, amountAsset / 100);
uint256 assetsReceived = vault.redeem(numShares, user, user);

uint256 assetsReceived = vault.redeem(expectedShares, user, user);
assertEq(vault.balanceOf(user), 0);
assertEq(assetsReceived, amountAsset);
}

function testDepositWithdraw(uint128 amountAsset) public {
function testDepositWithdraw(uint64 amountAsset) public {
vm.assume(amountAsset > 99);
// Using a uint128 since we multiply totalSupply by amountAsset in sharesFromTokens
// Using a uint128 makes sure the calculation will not overflow
// shares = assets * totalShares / totalAssets but totalShares will actually be bigger than a uint128
// so the `assets * totalShares` calc will overflow if using a uint128
address user = address(this);
asset.mint(user, amountAsset);
asset.approve(address(vault), type(uint256).max);

// If vault is empty, assets are converted to shares at 1:1e8 ratio
uint256 expectedShares = uint256(amountAsset) * 1e8; // cast to uint256 to prevent overflow

vm.expectEmit(true, true, true, true);
emit Deposit(address(this), address(this), amountAsset, amountAsset / 100);
emit Deposit(address(this), address(this), amountAsset, expectedShares);
vault.deposit(amountAsset, user);

// If vault is empty, assets are converted to shares at 100:1
assertEq(vault.balanceOf(user), amountAsset / 100);
assertEq(vault.balanceOf(user), expectedShares);
assertEq(asset.balanceOf(user), 0);

vm.expectEmit(true, true, true, true);
emit Withdraw(user, user, user, amountAsset, amountAsset / 100);
// vm.expectEmit(true, true, true, true);
// emit Withdraw(user, user, user, amountAsset, expectedShares);
// emit log_named_uint("expected shares: ", vault.convertToShares(uint(amountAsset)));
vault.withdraw(amountAsset, user, user);
assertEq(vault.balanceOf(user), 0);
assertEq(asset.balanceOf(user), amountAsset);
}

function testMint(uint128 amountAsset) public {
function testMint(uint64 amountAsset) public {
vm.assume(amountAsset > 99);
address user = address(this);
asset.mint(user, amountAsset);
asset.approve(address(vault), type(uint256).max);

uint256 numShares = amountAsset / 100;
vm.expectEmit(true, true, true, true);
emit Deposit(address(this), address(this), numShares * 100, numShares);
vault.mint(numShares, user);
// If vault is empty, assets are converted to shares at 1:1e8 ratio
uint256 expectedShares = uint256(amountAsset) * 1e8; // cast to uint256 to prevent overflow

// If vault is empty, assets are converted to shares at 100:1
assertEq(vault.balanceOf(user), numShares);
vm.expectEmit(true, true, true, true);
emit Deposit(address(this), address(this), amountAsset, expectedShares);
vault.mint(expectedShares, user);

// E.g. is amountAsset is 201, numShares is 2 and we actually have 1 in asset left
assertEq(asset.balanceOf(user), amountAsset - (numShares * 100));
assertEq(asset.balanceOf(address(vault)), numShares * 100);
assertEq(vault.balanceOf(user), expectedShares);
}

function testMinDeposit() public {
address user = address(this);
asset.mint(user, 100);
asset.approve(address(vault), type(uint256).max);

// shares = assets / 100. If you give less than 100 in assets we revert
// If we're minting zero shares we revert
vm.expectRevert("MIN_DEPOSIT_ERR");
vault.deposit(99, user);
vault.deposit(0, user);

vault.deposit(100, user);
}
Expand Down Expand Up @@ -407,7 +399,7 @@ contract L2VaultTest is TestPlus {
uint256 indexed pos, address indexed owner, address indexed receiver, uint256 amount
);

function testEmergencyWithdrawal(uint128 amountAsset) public {
function testEmergencyWithdrawal(uint64 amountAsset) public {
vm.assume(amountAsset > 99);
address user = address(this);
asset.mint(user, amountAsset);
Expand All @@ -422,10 +414,8 @@ contract L2VaultTest is TestPlus {
bytes32(uint256(amountAsset))
);

vm.startPrank(user);

vm.expectEmit(true, true, false, true);
emit EmergencyWithdrawalQueueEnqueue(1, user, user, vault.previewWithdraw(amountAsset));
vm.expectEmit(true, true, true, false);
emit EmergencyWithdrawalQueueEnqueue(1, user, user, vault.convertToShares(amountAsset));

// Trigger emergency withdrawal as vault doesn't have any asset.
vault.withdraw(amountAsset, user, user);
Expand All @@ -442,7 +432,7 @@ contract L2VaultTest is TestPlus {
assertEq(asset.balanceOf(user), amountAsset);
}

function testEmergencyWithdrawalWithRedeem(uint128 amountAsset) public {
function testEmergencyWithdrawalWithRedeem(uint64 amountAsset) public {
vm.assume(amountAsset > 99);
address user = address(this);
asset.mint(user, amountAsset);
Expand All @@ -457,9 +447,8 @@ contract L2VaultTest is TestPlus {
bytes32(uint256(amountAsset))
);

vm.startPrank(user);
uint256 numShares = vault.convertToShares(amountAsset);
vm.expectEmit(true, true, false, true);
vm.expectEmit(true, true, true, true);
emit EmergencyWithdrawalQueueEnqueue(1, user, user, numShares);

// Trigger emergency withdrawal as vault doesn't have any asset.
Expand Down Expand Up @@ -638,23 +627,18 @@ contract L2VaultTest is TestPlus {
function testDetailedPrice() public {
// This function should work even if there is nothing in the vault
L2Vault.Number memory price = vault.detailedPrice();
assertEq(price.num, 100 ** vault.decimals());

address user = address(this);
asset.mint(user, 2e18);
asset.approve(address(vault), type(uint256).max);
assertEq(price.num, 100 * 10 ** uint256(asset.decimals()));

vault.deposit(1e18, user);
asset.transfer(address(vault), 1e18);
asset.mint(address(vault), 2e18);

// initial price is $100, but if we increase tvl by two it will be 200
// initial price is $100, but if we increase tvl the price increases
L2Vault.Number memory price2 = vault.detailedPrice();
assertEq(price2.num, 200 * 10 ** vault.decimals());
assertTrue(price2.num > price.num);
}

function testSettingForwarder() public {
changePrank(governance);
address newForwarder = 0x8f954E7D7ec3A31D9568316fb0F472B03fc2a7d5;
address newForwarder = makeAddr("new_forwarder");
vault.setTrustedForwarder(newForwarder);
assertEq(vault.trustedForwarder(), newForwarder);

Expand Down
2 changes: 1 addition & 1 deletion src/test/Router.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ contract RouterTest is TestPlus {
router.approve(token, address(vault), 2e6);
router.approve(token, address(basket), 2e6);
router.multicall(data);
assert(vault.balanceOf(user) == 1e6 / 100);
assert(vault.balanceOf(user) > 0);
assert(basket.balanceOf(user) > 0);
}
}