PoolTogether - SAQ's results

Summary

Gas Optimization

Gas Optimizations

[G-01] Use assembly to emit events

We can use assembly to emit events efficiently by utilizing scratch space and the free memory pointer. This will allow us to potentially avoid memory expansion costs. Note: In order to do this optimization safely, we will need to cache and restore the free memory pointer.

file:/src/PrizeVault.sol

 562       emit Sponsor(_owner, _assets, _shares);

 621        emit ClaimYieldFeeShares(msg.sender, _shares);

 697        emit TransferYieldOut(msg.sender, _tokenOut, _receiver, _amountOut, _yieldFee);

 747        emit LiquidationPairSet(address(this), address(_liquidationPair));

 876        emit Deposit(_caller, _receiver, _assets, _shares);

 909        emit Withdraw(_caller, _receiver, _owner, _assets, _shares);

 952       emit YieldFeePercentageSet(_yieldFeePercentage);

 960        emit YieldFeeRecipientSet(_yieldFeeRecipient);

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L562

file:/src/TwabERC20.sol

 78       emit Transfer(address(0), _receiver, _amount);

 89        emit Transfer(_owner, address(0), _amount);

 102        emit Transfer(_from, _to, _amount);

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/TwabERC20.sol#L78

file:/src/abstract/Claimable.sol

131        emit ClaimerSet(_claimer);

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/abstract/Claimable.sol#L131

file:/src/abstract/HookManager.sol

31        emit SetHooks(msg.sender, hooks);

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/abstract/HookManager.sol#L31

[G-02] Use hardcode address instead address(this)

Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry’s script.sol and solmate’s LibRlp.sol contracts can help achieve this.

file:/src/PrizeVault.sol

337        return yieldVault.convertToAssets(yieldVault.balanceOf(address(this))) + _asset.balanceOf(address
(this));

382       uint256 _latentBalance = _asset.balanceOf(address(this));

383       uint256 _maxYieldVaultDeposit = yieldVault.maxDeposit(address(this));

405        uint256 _maxWithdraw = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));

416        uint256 _maxWithdraw = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));

540         IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s);

558        if (twabController.delegateOf(address(this), _owner) != SPONSORSHIP_ADDRESS) {

639            _maxAmountOut = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));

726        return (_tokenOut == address(_asset) || _tokenOut == address(this)) && _liquidationPair == liquidationPair;

861        uint256 _assetsWithDust = _asset.balanceOf(address(this));

866        uint256 _assetsUsed = yieldVault.mint(_yieldVaultShares, address(this));

922        return yieldVault.convertToAssets(yieldVault.maxRedeem(address(this)));

936            yieldVault.redeem(_yieldVaultShares, address(this), address(this));
      

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L337

file:/src/TwabERC20.sol

 59       return twabController.balanceOf(address(this), _account);

 64        return twabController.totalSupply(address(this));

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/TwabERC20.sol#L59

[G-03] Using unchecked blocks to save gas

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

file:/src/PrizeVault.sol

 617        yieldFeeBalance -= _yieldFeeBalance;

 647      uint256 _liquidYield = 
 648          _availableYieldBalance(totalAssets(), _totalDebt(_totalSupply))
 649          .mulDiv(FEE_PRECISION - yieldFeePercentage, FEE_PRECISION);


 675            _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut;

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L617

[G-04] Use assembly for math (add, sub, mul, div)

Use assembly for math instead of Solidity. You can check for overflow/underflow in assembly to ensure safety. If using Solidity versions < 0.8.0 and you are using Safemath, you can gain significant gas savings by using assembly to calculate values and checking for overflow/underflow.

file:/src/PrizeVault.sol

416         uint256 _maxWithdraw = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));

617        yieldFeeBalance -= _yieldFeeBalance;

639            _maxAmountOut = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L416

[G-05] Not using the named return variable when a function returns, wastes deployment gas

When you execute a function that returns values in Solidity, the EVM still performs the necessary operations to execute and return those values. This includes the cost of allocating memory and packing the return values. If the returned values are not utilized, it can be seen as wasteful since you are incurring gas costs for operations that have no effect.

file:/src/PrizeVaultFactory.sol

    ) external returns (PrizeVault) {

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVaultFactory.sol#L101

[G-06] Use assembly for small keccak256 hashes, in order to save gas

If the arguments to the encode call can fit into the scratch space (two words or fewer), then it's more efficient to use assembly to generate the hash (80 gas)

File: /src/PrizeVaultFactory.sol

 103           salt: keccak256(abi.encode(msg.sender, deployerNonces[msg.sender]++))

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVaultFactory.sol#L103

[G-07] Use assembly to validate msg.sender

file:/src/abstract/Claimable.sol

 53       if (msg.sender != claimer) revert CallerNotClaimer(msg.sender, claimer);

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/abstract/Claimable.sol#L53

[G-08] Use Mappings Instead of Arrays

When we say "use mappings instead of arrays," we mean that when storing and accessing data in a smart contract, it's more gas-efficient to use mappings instead of arrays. This is because mappings do not require iteration to find a specific element, whereas arrays do.

file:/src/PrizeVaultFactory.so

66   PrizeVault[] public allVaults;

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVaultFactory.sol#L66

[G-09] Sort Solidity operations using short-circuit mode

file:/src/PrizeVault.sol

 776       if (success && encodedDecimals.length >= 32) {

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L776

[G-10] Before transfer of some functions, we should check some variables for possible gas save

Before transfer, we should check for amount being 0 so the function doesn't run when its not gonna do anything:

file:/src/PrizeVault.sol

  939          _asset.transfer(_receiver, _assets);

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L939

file:/src/TwabERC20.sol

  78      emit Transfer(address(0), _receiver, _amount);

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/TwabERC20.sol#L78

[G-11] Use assembly to check for address(0)

file: /src/PrizeVault.sol

 300       if (address(yieldVault_) == address(0)) revert YieldVaultZeroAddress();

 301        if (owner_ == address(0)) revert OwnerZeroAddress();
 
 743        if (address(_liquidationPair) == address(0)) revert LPZeroAddress();

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/PrizeVault.sol#L300

[G-12] Using PRIVATE rather than PUBLIC FOR Immutable, Saves Gas

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table

file: /src/TwabERC20.sol

 26   TwabController public immutable twabController;

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/TwabERC20.sol#L26C27-L26

file:/src/abstract/Claimable.sol

 24   PrizePool public immutable prizePool;

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/abstract/Claimable.sol#L24

[G-13] Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract's gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

file:/src/TwabERC20.sol

101        twabController.transfer(_from, _to, SafeCast.toUint96(_amount));

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/TwabERC20.sol#L101

File:/src/abstract/Claimable.sol

 21   uint24 public constant HOOK_GAS = 150_000;

https://github.com/code-423n4/2024-03-pooltogether/blob/480d58b9e8611c13587f28811864aea138a0021a/pt-v5-vault/src/abstract/Claimable.sol#L21

Summary

Analysis Issue Report

[File-1] PrizeVault.sol

The bellow issues related to the File ( Link )

Github-Link

<details> <summary>see instances</summary>

Admin Abuse Risks:

• Permit Functionality: The depositWithPermit function allows users to approve and deposit assets using a permit. However, the comment mentions a potential risk related to the lack of a receiver parameter in the permit, allowing potential misuse.

• Access Controls: The contract includes several functions with the onlyOwner modifier, indicating that certain actions can only be performed by the owner. Ensure that proper access controls are in place and appropriately tested

Systemic Risks:

• TWAB Controller Dependency: The contract relies on an external TWAB controller (twabController). Ensure that the integration with this controller is secure and well-audited, as it seems to impact critical functions like sponsorship.

• Yield Fee Handling: The contract implements a yield fee mechanism. Ensure that the fee calculations and distributions are well-tested to prevent any unintended consequences.

Technical Risks:

• ERC-20 Approval Risks: The contract uses ERC-20's approve function, and it is crucial to handle allowances securely to prevent potential vulnerabilities like front-running attacks.

• Reentrancy Consideration: The code mentions considerations related to reentrancy during asset transfers. Make sure that reentrancy risks are thoroughly mitigated, especially during ERC-777 and ERC-20 transfers.

Integration Risks:

• External Contracts: The contract interacts with external contracts, such as yieldVault and prizePool. Ensure that these contracts are trustworthy and correctly integrated, considering potential vulnerabilities.

• Liquidation Logic: The ILiquidationSource interface and liquidation-related functions introduce dependencies on external systems. Assess and test the integration with these systems to avoid unforeseen issues.

Non-Standard Token Risks:

• ERC-777 Considerations: The contract mentions handling ERC-777 tokens and notes potential reentrancy issues. Ensure the contract correctly handles ERC-777 tokens, considering their unique features.

• Permit Functionality: The use of permit functionality may be non-standard, depending on the specific ERC-20 implementation. Ensure compatibility with popular ERC-20 token standards.

</details>

[File-2] PrizeVaultFactory.sol

The bellow issues related to the File ( Link )

Github-Link

<details> <summary>see instances</summary>

Admin Abuse Risks:

• Nonce Handling: The contract utilizes nonces for address-based salts during vault deployment (keccak256(abi.encode(msg.sender, deployerNonces[msg.sender]++))). While nonces generally prevent replay attacks, ensure that the nonce management is secure and doesn't introduce vulnerabilities.

• Token Transfer: The deployVault function transfers assets to the newly deployed vault to fill the yield buffer. Admins must be cautious about the management of these funds, and any vulnerability in this process could lead to potential abuse.

Systemic Risks:

• Dependency on External Contracts: The contract relies on external contracts (PrizeVault, IERC4626, PrizePool) for various functionalities. Ensure that these contracts are secure and well-audited, as any vulnerabilities in them could impact the overall system.

• Relying on External Yield Vault: The contract relies on an external ERC4626 yield vault (_yieldVault). Ensure that the integration with this yield vault is secure, and consider potential risks associated with the yield generation process.

Technical Risks:

• CREATE2 Usage: The contract uses CREATE2 to deploy new PrizeVault instances. While this is a standard practice, ensure that the implementation is correct, and the risk of collisions is adequately mitigated.

• Address Calculation for Salt: The salt for CREATE2 is generated using keccak256(abi.encode(msg.sender, deployerNonces[msg.sender]++)). Validate that this salt calculation is robust and doesn't introduce any vulnerabilities.

Integration Risks:

• ERC-4626 Integration: The contract relies on an external ERC-4626 yield vault (_yieldVault). Ensure proper integration with this contract and assess potential risks associated with the yield generation process.

• PrizePool Integration: The contract interacts with an external PrizePool contract (_prizePool). Verify that the integration is secure and well-tested, considering potential implications for prize computations.

Non-Standard Token Risks:

• Token Transfer: The deployVault function involves the transfer of tokens (IERC20(_vault.asset()).transferFrom(msg.sender, address(_vault), YIELD_BUFFER)). Ensure that this token transfer is secure and won't result in any unexpected behavior.

Yield Fee Handling: The contract includes parameters related to yield fees (_yieldFeeRecipient, _yieldFeePercentage). Assess the implementation to ensure correct handling of yield fees and prevent any unintended consequences.

Recommendations:

1. Security Audits:

   • Conduct thorough security audits on external contracts (PrizeVault, IERC4626, PrizePool).

2. Nonce Security:

   • Implement secure nonce management to prevent replay attacks.

3. Token Transfers:

   • Review and test token transfer logic in the deployVault function for robustness.

4. Yield Vault Integration:

   • Ensure secure integration with the external ERC4626 yield vault.

5. CREATE2 Usage:

   • Verify correctness in using CREATE2 for deploying PrizeVault instances.

</details>

[File-3] TwabERC20.sol

The bellow issues related to the File ( Link )

Github-Link

<details> <summary>see instances</summary>

Admin Abuse Risks:

• No explicit admin controls: The contract lacks explicit functions or modifiers for administrative actions. Admin abuse risks are mitigated by adhering to the standard ERC20 functions and relying on the TwabController.

Systemic Risks:

• TwabController Dependency: The contract is tightly coupled with the TwabController. Systemic risks may arise if the TwabController has vulnerabilities or malfunctions. Thoroughly audit the TwabController for robustness.

Technical Risks:

• Gas Efficiency: Gas efficiency is prioritized by using uint96 for balances. Review potential gas implications and ensure minting doesn't exceed uint96 limits, causing failed transactions.

Integration Risks:

• ERC20 Compliance: The contract inherits from OpenZeppelin's ERC20 and ERC20Permit, ensuring standard ERC20 functionality. Integration risks are low, but thoroughly test interactions with external systems.

Non-Standard Token Risks:

• TwabController Dependency: The contract relies on a specific balance tracking mechanism (TwabController). Ensure compatibility with other systems, and assess risks associated with potential changes or upgrades to the TwabController.

Recommendations:

1. TwabController Audit:

   • Conduct a thorough audit of the TwabController to identify and address any vulnerabilities.

2. Gas Efficiency Testing:

   • Test the gas efficiency of the contract, especially minting functions, to ensure they stay within uint96 limits.

3. Integration Testing:

   • Conduct extensive integration testing to verify seamless interaction with external systems, especially with the TwabController.

4. Documentation:

   • Provide clear documentation on the integration process and dependencies, especially regarding the TwabController.

5. Security Best Practices:

   • Follow security best practices in contract development and consider additional security measures if necessary.

</details>

[File-4] Claimable.sol

The bellow issues related to the File ( Link )

Github-Link

<details> <summary>see instances</summary>

Admin Abuse Risks:

• Claimer Authority: Admin abuse risks are present due to the claimer variable, which determines the address allowed to claim prizes. Ensure that the address is set securely and that its authority is well-defined.

Systemic Risks:

• PrizePool Dependency: The contract is tightly coupled with the PrizePool contract. Systemic risks may arise if the PrizePool has vulnerabilities or malfunctions. A comprehensive audit of the PrizePool contract is crucial.

Technical Risks:

• Hook Execution: The contract implements hooks for before and after prize claim actions. Ensure that these hooks are implemented securely, as any issues in their execution can impact prize claims.

Integration Risks:

• ClaimPrize Function: The claimPrize function interacts with external systems, specifically the PrizePool. Integration risks may arise if the PrizePool contract behavior changes. Test thoroughly for seamless interaction.

Non-Standard Token Risks:

• No Non-Standard Token Risks Detected: The contract does not involve non-standard tokens.

Recommendations:

1. Claimer Authentication:
   • Implement secure mechanisms to set and manage the claimer address, potentially utilizing multi-signature schemes or time-lock mechanisms to minimize admin abuse risks.
2. PrizePool Audit:
   • Conduct a thorough audit of the PrizePool contract to identify and address any vulnerabilities. Ensure that the PrizePool contract is robust and secure.
3. Hook Implementation Review:
   • Review the implementation of before and after claim hooks. Ensure that the hooks are securely designed and do not introduce vulnerabilities.
4. Integration Testing:
   • Test the contract's interaction with the PrizePool thoroughly. Simulate various scenarios, including potential changes in PrizePool behavior, to ensure smooth integration.
5. Documentation:
   • Provide clear documentation on the purpose and usage of the contract, including details on the claimer address and the hook mechanisms.

</details>

[File-5] HookManager.sol

The bellow issues related to the File ( Link )

Github-Link

<details> <summary>see instances</summary>

Admin Abuse Risks:

• SetHooks Functionality: There's a potential risk of admin abuse as the setHooks function allows any account to set hooks for themselves. Ensure that only authorized users can set hooks to prevent misuse.

Systemic Risks:

• Dependency on External Hooks: The contract introduces a system where individual accounts can set hooks. Systemic risks may arise if the hook execution mechanism is not well-secured or if it introduces vulnerabilities.

Technical Risks:

• Hook Storage: Storing hooks in a mapping (_hooks) could lead to higher gas costs if the mapping becomes too large. Ensure that the impact on gas consumption is acceptable.

Integration Risks:

• External Systems Interaction: If the hooks interact with external systems, there is an integration risk. Ensure that the external systems are secure and that the interaction does not introduce vulnerabilities.

Non-Standard Token Risks:

• No Non-Standard Token Risks Detected: The contract does not involve non-standard tokens.

Recommendations:

1. Access Control:
   • Implement access control mechanisms to restrict who can call the setHooks function. This helps prevent unauthorized users from setting hooks.
2. Security Audits:
   • Conduct a security audit, especially if the hooks interact with external systems. Verify that the hooks and their execution are secure and do not expose the contract to vulnerabilities.
3. Gas Consumption Consideration:
   • Evaluate the potential impact on gas consumption due to the storage of hooks in a mapping. Consider gas-efficient alternatives if the mapping size becomes a concern.
4. Documentation:
   • Provide comprehensive documentation on the purpose of hooks, how users can set them, and any potential risks associated with hook execution.

</details>

Time spent:

5 hours