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

12 stars 7 forks source link

First depositor can break share #261

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L864-#L874

Vulnerability details

At first deposit to vault, exchange rate will be 1 as developer commented: https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1165

* @dev We start with an exchange rate of 1 which is equal to 1 underlying asset unit.

Along with there is no mechanism to ignore deposit that have share = 0, it will open for inflation attack vector:

Attack scenario: 1, Attacker observate creation of Vault creation. 2, Right after deposit, attacker mint themself one share. At that time _totalAsset()=1 and _totalSupply()=1. 3, Attacker front-run the deposit of victim who want to deposit x asset 4, Attacker inflates in front of the victim with x asset. At that time, _totalAsset()= x + 1 and _totalSupply()=1. 5, Victim transaction complete, victim got x*(x+1) = 0 shares due to rounding dound 6, Attacker burn shares and get profit

Even if there is mechanism that revert if total shares that victim can receive equal to 0, there is also attack vector to make attacker profit. suppose x is an even number, repeat all step like above, but this time, attacker inflates with x/2 asset in step 4. at that time, victim got x*(x/2+1) = 1 shares, which is equal to attacker. After that, attacker burn shares and receive back 3x/4 asset, profit x/4 asset.

Impact

First victim who deposit to Vault will lose all money without gaining anything.

Tools Used

Manual review.

Recommended Mitigation Steps

Consult this OpenZeppelin Github issue: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3706

Assessed type

Other

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid