Platform: Code4rena
Start Date: 04/03/2024
Pot Size: $36,500 USDC
Total HM: 9
Participants: 80
Period: 7 days
Judge: hansfriese
Total Solo HM: 2
Id: 332
League: ETH
Rank: 15/80
Findings: 2
Award: $378.99
🌟 Selected for report: 1
🚀 Solo Findings: 0
187.3764 USDC - $187.38
| Issue | Instances | |
|---|---|---|
| [L-01] | Centralization risk for privileged functions | 6 |
| [L-02] | Subtraction in unchecked block is unsafe | 1 |
| [L-03] | decimals() is not part of the ERC20 standard | 1 |
| [L-04] | Some tokens may revert on large transfers | 2 |
| [L-05] | Code does not follow the best practice of check-effects-interaction | 1 |
| [L-06] | Use abi.encodeCall() instead of abi.encodeWithSignature()/abi.encodeWithSelector() | 1 |
| [L-07] | Upgradable contracts not taken into account | 3 |
| [L-08] | Lack of index element validation in external/public function | 5 |
| [L-09] | Consider checking that transfer to address(this) is disabled | 3 |
| [L-10] | Unchecked Return Values of the approve() Function | 2 |
| [L-11] | Use forceApprove/safeIncreaseAllowance from SafeERC20 instead of approve/safeApprove | 1 |
| [L-12] | Unsafe use of transfer()/transferFrom() with IERC20 | 2 |
| [L-13] | Unchecked Return Values of transfer()/transferFrom() | 2 |
| [L-14] | Solidity version 0.8.20 may not work on other chains due to PUSH0 | 6 |
| [L-15] | Missing checks for address(0x0) when updating address state variables | 1 |
| [L-16] | Lack of Parameter Validation in Constructor/Initializer | 1 |
| [L-17] | Owner can renounce Ownership | 1 |
| [L-18] | Missing Contract-Existence Checks Before Low-Level Calls | 1 |
| [L-19] | Events may be emitted out of order due to reentrancy | 3 |
| [L-20] | Critical functions should be a two step procedure | 5 |
| [L-21] | Array is push()ed but not pop()ed | 1 |
| [L-22] | Consider implementing two-step procedure for updating protocol addresses | 4 |
| [L-23] | Function Parameters in Public Accessible Functions Need address(0) Check | 1 |
| [L-24] | Missing address(0) Check in Constructor | 2 |
| [L-25] | Functions calling tokens with transfer hooks are missing reentrancy guards | 4 |
| [L-26] | Consider Using Ownable2Step rather than Ownable | 2 |
Total: 62 instances of 26 issues
| Issue | Instances | |
|---|---|---|
| [N-01] | Loss of precision | 1 |
| [N-02] | Non-library/interface files should use fixed compiler versions, not floating ones | 5 |
| [N-03] | Consider splitting long calculations | 1 |
| [N-04] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, for readability | 2 |
| [N-05] | Consider using a struct rather than having many function input parameters | 5 |
| [N-06] | Consider using descriptive constants when passing zero as a function argument | 1 |
| [N-07] | Custom error has no error details | 14 |
| [N-08] | Consider using OpenZeppelin's SafeCast for any casting | 1 |
| [N-09] | Consider emitting an event at the end of the constructor | 3 |
| [N-10] | Consider providing a ranged getter for array state variables | 1 |
| [N-11] | Consider using named returns | 39 |
| [N-12] | Consider using named function arguments | 4 |
| [N-13] | Events are missing sender information | 3 |
| [N-14] | Leverage Recent Solidity Features with 0.8.23 | 1 |
| [N-15] | NatSpec: Function @param tag is missing | 1 |
| [N-16] | High cyclomatic complexity | 1 |
| [N-17] | Contract uses both require()/revert() as well as custom errors | 2 |
| [N-18] | Solidity Version Too Recent to be Trusted | 5 |
| [N-19] | Inconsistent spacing in comments | 140 |
| [N-20] | Unused error definition | 1 |
| [N-21] | Critical system parameter changes should be behind a timelock | 4 |
| [N-22] | Outdated Solidity Version | 1 |
| [N-23] | Ensure Non-Empty Check for Bytes in Function Parameters | 2 |
| [N-24] | Function/Constructor Argument Names Not in mixedCase | 44 |
| [N-25] | Upgrade openzeppelin to the Latest Version - 5.0.0 | 8 |
| [N-26] | Duplicated require()/revert() checks should be refactored to a modifier or function | 2 |
| [N-27] | Event is not properly indexed | 4 |
| [N-28] | Consider Using safePermit() Instead of permit() | 1 |
| [N-29] | Consider Adding Emergency-Stop Functionality | 2 |
| [N-30] | Consider adding a block/deny-list | 2 |
| [N-31] | Non-uppercase/Constant-case Naming for immutable Variables | 6 |
| [N-32] | Insufficient Comments in Complex Functions | 1 |
| [N-33] | constants should be defined rather than using magic numbers | 2 |
| [N-34] | Consider moving msg.sender checks to a common authorization modifier | 1 |
| [N-35] | NatSpec: Contract declarations should have @author tag | 1 |
| [N-36] | Consider defining system-wide constants in a single file | 4 |
| [N-37] | Add inline comments for unnamed variables | 4 |
| [N-38] | Consider making contracts Upgradeable | 5 |
| [N-39] | Events that mark critical parameter changes should contain both the old and the new value | 2 |
| [N-40] | Style guide: Contract does not follow the Solidity style guide's suggested layout ordering | 2 |
| [N-41] | Style guide: Control structures do not follow the Solidity Style Guide | 18 |
| [N-42] | Constants in comparisons should appear on the left side | 10 |
| [N-43] | NatSpec: Contract declarations should have @notice tag | 1 |
| [N-44] | Long functions should be refactored into multiple, smaller, functions | 1 |
| [N-45] | NatSpec: Contract declarations should have @dev tags | 2 |
| [N-46] | public functions not called by the contract should be declared external instead | 6 |
| [N-47] | else-block not required | 7 |
| [N-48] | Style guide: Function ordering does not follow the Solidity style guide | 8 |
| [N-49] | Style guide: Extraneous whitespace | 47 |
| [N-50] | Unused import | 1 |
| [N-51] | if-statement can be converted to a ternary | 1 |
| [N-52] | Consider using named mappings | 1 |
| [N-53] | Use of override is unnecessary | 6 |
| [N-54] | Setters should prevent re-setting of the same value | 7 |
| [N-55] | NatSpec: Contract declarations should have @title tags | 1 |
| [N-56] | Contracts should have full test coverage | 1 |
| [N-57] | Large or complicated code bases should implement invariant tests | 1 |
| [N-58] | Implement Formal Verification Proofs to Improve Security | 1 |
Total: 449 instances of 58 issues
Functions that grant centralized privileges introduce a notable security risk. Such functions can act as a single point of vulnerability, especially if they control critical operations or access to sensitive data.
If the account or entity with these privileges gets compromised, it could lead to the unauthorized alteration of the contract's state, asset misappropriation, or other malicious activities. It's essential to implement robust access controls, frequent audits, and potentially decentralize critical functions to mitigate this risk.
<details> <summary><i>6 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 703: function verifyTokensIn( address _tokenIn, uint256 _amountIn, bytes calldata ) external onlyLiquidationPair 735: function setClaimer(address _claimer) external onlyOwner 742: function setLiquidationPair(address _liquidationPair) external onlyOwner 753: function setYieldFeePercentage(uint32 _yieldFeePercentage) external onlyOwner 759: function setYieldFeeRecipient(address _yieldFeeRecipient) external onlyOwner
</details>File: pt-v5-vault/src/abstract/Claimable.sol 76: function claimPrize( address _winner, uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256)
unchecked block is unsafePerforming subtraction within an unchecked block poses a risk of silent underflow, leading to unintended behavior or vulnerabilities. Without proper value checks, the subtraction could result in values that are lower than expected, potentially impacting the logic and security of the contract.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 800: return type(uint96).max - _totalSupply;
decimals() is not part of the ERC20 standardIt's often used, but decimals() is not part of the original ERC-20 standard. It was added later as an optional extension.
Some valid ERC-20 tokens do not support this function, so casting all tokens to an interface that includes decimals() can be unsafe.
Consider revising your code to account for tokens that may not implement the decimals() function.
</details>File: pt-v5-vault/src/PrizeVault.sol 774: abi.encodeWithSelector(IERC20Metadata.decimals.selector)
Tokens like:
have limit set by uint96.
Transfers that approach or exceed this limit will revert. Be cautious of such transfers, especially if batching is not implemented to handle these scenarios.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 854: _asset.safeTransferFrom( _caller, address(this), _assets ) 939: _asset.transfer(_receiver, _assets)
Code should follow the best-practice of check-effects-interaction, where state variables are updated before any external calls are made. Doing so prevents a large class of reentrancy bugs.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVaultFactory.sol /// @audit - State change after external call: `IERC20(_vault.asset()).transferFrom(msg.sender, address(_vault), YIELD_BUFFER)` 121: deployedVaults[address(_vault)] = true
abi.encodeCall() instead of abi.encodeWithSignature()/abi.encodeWithSelector()As of Solidity version 0.8.11, abi.encodeCall() provides type-safe encoding utility, a notable enhancement over abi.encodeWithSelector().
While the latter can prevent typographical errors, it doesn't provide type checking.
The use of abi.encodeCall() introduces type safety at compile time, leading to safer and more robust smart contracts.
Read more about type safety
Please note that adopting abi.encodeCall() requires bumping the Solidity pragma, which could introduce breaking changes.
</details>File: pt-v5-vault/src/PrizeVault.sol 774: abi.encodeWithSelector(IERC20Metadata.decimals.selector)
In the realm of blockchain development, it's crucial to consider the impact of upgradable contracts, especially when handling token addresses through interfaces like IERC20. These contracts can evolve over time, potentially altering their behavior or interface. Such changes may lead to compatibility issues or security vulnerabilities in the protocol that relies on them.
<details> <summary><i>3 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 303: IERC20 asset_ = IERC20(yieldVault_.asset()); 540: IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s);
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol 118: IERC20(_vault.asset()).transferFrom(msg.sender, address(_vault), YIELD_BUFFER);
There's no validation to check whether the index element provided as an argument actually exists in the call. This omission could lead to unintended behavior if an element that does not exist in the call is passed to the function.
The function should validate that the provided index element exists in the call before proceeding.
Without this validation, the function could cause unintended behaviour as it will call an non-existing index element. This could lead to inconsistencies in data and potentially affect the integrity of the call structure.
<details> <summary><i>5 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/abstract/Claimable.sol 85: _hooks[_winner] 86: _hooks[_winner] 108: _hooks[_winner] 109: _hooks[_winner]
</details>File: pt-v5-vault/src/abstract/HookManager.sol 23: _hooks[account]
address(this) is disabledFunctions allowing users to specify the receiving address should check whether the referenced address is not address(this).
This would prevent tokens to remain lock inside the contract due to an incorrect transfer or mint.
</details>File: pt-v5-vault/src/PrizeVault.sol 692: _mint(_receiver, _amountOut) 872: _mint(_receiver, _shares) 939: _asset.transfer(_receiver, _assets)
approve() FunctionThe unchecked return value of the approve() method can potentially cause transaction failures to go unnoticed in your contract.
Some IERC20 token implementations utilize boolean return values to indicate transaction failures, instead of relying on the revert() function. If the return value of the approve() method isn't appropriately verified, transactions may seemingly proceed even when the necessary token approvals have not been appropriately executed.
As a safeguard, it is crucial to consistently verify the return values of these methods. This precaution helps to ensure the correct execution of token approvals, maintaining the integrity of transaction operations and reducing the risk of unnoticed transaction failures.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 862: _asset.approve(address(yieldVault), _assetsWithDust); 869: _asset.approve(address(yieldVault), 0);
forceApprove/safeIncreaseAllowance from SafeERC20 instead of approve/safeApproveCertain tokens, exemplified by USDT, have quirks where adjusting an allowance directly from a non-zero value can be problematic. For such tokens, it's a common requirement to first reset the allowance to zero before setting a new value. Failing to do so can result in transaction reverts, disrupting protocol functionality.
To gracefully handle this, consider using the forceApprove method from OpenZeppelin's SafeERC20 library. This method ensures the contract's allowance towards a spender is set to the specified value. If the token doesn't return a value, the non-reverting calls are assumed successful, offering a seamless solution for tokens like USDT.
Since safeIncreaseAllowance calls forceApprove internally, it's also a viable alternative.
Reference the SafeERC20 Library for more details.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 862: _asset.approve(address(yieldVault), _assetsWithDust);
transfer()/transferFrom() with IERC20Not all tokens adhere strictly to the ERC20 standard, even if they are largely accepted as compliant. A prominent example is Tether (USDT) on L1, where the transfer() and transferFrom() functions do not return booleans, contrary to the standard's requirements. Instead, these functions provide no return value at all. This discrepancy can lead to issues when such tokens are cast to IERC20: their non-standard function signatures can cause calls to revert. As a safer alternative, consider using OpenZeppelin's SafeERC20 library, which offers safeTransfer() and safeTransferFrom() functions to address these incompatibilities.
<details> <summary><i>2 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 939: _asset.transfer(_receiver, _assets);
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol 118: IERC20(_vault.asset()).transferFrom(msg.sender, address(_vault), YIELD_BUFFER);
transfer()/transferFrom()transfer() or transferFrom() methods without checking their return values.
Some IERC20 implementations use these boolean return values to signal transaction failures instead of using revert().
Not checking these values could allow transactions to proceed without the intended token transfers.
It is recommended to always check the return values of these methods.
File: pt-v5-vault/src/PrizeVault.sol 939: _asset.transfer(_receiver, _assets);
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol 118: IERC20(_vault.asset()).transferFrom(msg.sender, address(_vault), YIELD_BUFFER);
PUSH0Solidity 0.8.20, by default, targets the Shanghai version of the EVM, which includes the new PUSH0 opcode.
However, this opcode may not yet be implemented on all Layer-2 chains.
For instance Arbitrum does not yet support PUSH0. See Documentation for more information.
As such, deploying contracts compiled with Solidity 0.8.20 on these chains could lead to failures.
To avoid this, consider specifying an older EVM version as the target, or ensure that the Layer-2 chain used supports the Shanghai EVM.
<details> <summary><i>6 issue instances in 6 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 2: pragma solidity ^0.8.24;
File: pt-v5-vault/src/PrizeVaultFactory.sol 2: pragma solidity ^0.8.24;
File: pt-v5-vault/src/TwabERC20.sol 2: pragma solidity ^0.8.24;
File: pt-v5-vault/src/abstract/Claimable.sol 2: pragma solidity ^0.8.24;
File: pt-v5-vault/src/abstract/HookManager.sol 2: pragma solidity ^0.8.0;
</details>File: pt-v5-vault/src/interfaces/IVaultHooks.sol 2: pragma solidity ^0.8.24;
address(0x0) when updating address state variablesState variables that are of type address should always be checked to ensure that they are not being assigned the null address (address(0x0)).
A null address often implies an error or omission in the code.
Without proper checks, such a scenario can lead to unexpected behavior and potential vulnerabilities in the smart contract.
Therefore, it is considered good practice to implement checks for address(0x0) prior to assigning new values to address state variables.
</details>File: pt-v5-vault/src/PrizeVault.sol 959: yieldFeeRecipient = _yieldFeeRecipient;
The constructor/initializer doesn't validate input parameters before assigning them to state variables. It is crucial to ensure that input parameters meet certain conditions to avoid unexpected states or behaviors in the contract. Consider adding appropriate checks or constraints.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol /// @audit `yieldFeePercentage_` is not validated before use /// @audit `yieldBuffer_` is not validated before use 289: constructor( string memory name_, string memory symbol_, IERC4626 yieldVault_, PrizePool prizePool_, address claimer_, address yieldFeeRecipient_, uint32 yieldFeePercentage_, uint256 yieldBuffer_, address owner_ ) TwabERC20(name_, symbol_, prizePool_.twabController()) Claimable(prizePool_, claimer_) Ownable(owner_) {
The contract uses solady/OpenZeppelin's Ownable(Upgradable).sol and contains onlyOwner functions.
Consider implementing custom logic for renounceOwnership() to handle renouncing ownership.
onlyOwner functions:
</details>File: pt-v5-vault/src/PrizeVault.sol 8: import { Ownable } from "owner-manager-contracts/Ownable.sol";
When making low-level calls, it's crucial to ensure the existence of the contract at the specified address. If the contract doesn't exist at the given address, low-level calls will still return success, potentially causing errors in the code execution. Therefore, alongside zero-address checks, adding an additional check to verify that <address>.code.length > 0 before making low-level calls would be recommended.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 773: (bool success, bytes memory encodedDecimals) = address(asset_).staticcall(
It's essential to ensure that events follow the best practice of check-effects-interaction and are emitted before any external calls to prevent out-of-order events due to reentrancy. Emitting events post external interactions may cause them to be out of order due to reentrancy, which can be misleading or erroneous for event listeners. Refer to the Solidity Documentation for best practices.
<details> <summary><i>3 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol /// @audit `safeTransferFrom()` before `Deposit` event emit 875: emit Deposit(_caller, _receiver, _assets, _shares);
File: pt-v5-vault/src/PrizeVaultFactory.sol /// @audit `transferFrom()` before `NewPrizeVault` event emit 122: emit NewPrizeVault( _vault, _yieldVault, _prizePool, _name, _symbol );
</details>File: pt-v5-vault/src/TwabERC20.sol /// @audit `transfer()` before `Transfer` event emit 102: emit Transfer(_from, _to, _amount);
Critical functions in Solidity contracts should follow a two-step procedure to enhance security, minimize human error, and ensure proper access control. By dividing sensitive operations into distinct phases, such as initiation and confirmation, developers can introduce a safeguard against unintended actions or unauthorized access.
<details> <summary><i>5 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 735: function setClaimer(address _claimer) external onlyOwner { 742: function setLiquidationPair(address _liquidationPair) external onlyOwner { 753: function setYieldFeePercentage(uint32 _yieldFeePercentage) external onlyOwner { 759: function setYieldFeeRecipient(address _yieldFeeRecipient) external onlyOwner {
</details>File: pt-v5-vault/src/abstract/HookManager.sol 29: function setHooks(VaultHooks calldata hooks) external {
push()ed but not pop()edArrays in the contract have entries added using the push method, but no corresponding pop method is found to remove them.
Ensure to consider whether old entries need to be removed or if there should be a maximum array length.
Cases with specific potential issues might be reported separately under different issue categories.
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol 120: allVaults.push(_vault);
Direct state address changes in a function can be risky, as they don't allow for a verification step before the change is made. It's safer to implement a two-step process where the new address is first proposed, then later confirmed, allowing for more control and the chance to catch errors or malicious activity.
<details> <summary><i>4 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 736: _setClaimer(_claimer); 744: liquidationPair = _liquidationPair; 760: _setYieldFeeRecipient(_yieldFeeRecipient); 959: yieldFeeRecipient = _yieldFeeRecipient;
address(0) CheckParameters of type address in your functions should be checked to ensure that they are not assigned the null address (address(0x0)).
Failure to validate these parameters can lead to transaction reverts, wasted gas, the need for transaction resubmission, and may even require redeployment of contracts within the protocol in certain situations.
Implement checks for address(0x0) to avoid these potential issues.
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol /// @audit `_claimer` parameter without address(0) check /// @audit `_yieldFeeRecipient` parameter without address(0) check /// @audit `_owner` parameter without address(0) check 92: function deployVault( string memory _name, string memory _symbol, IERC4626 _yieldVault, PrizePool _prizePool, address _claimer, address _yieldFeeRecipient, uint32 _yieldFeePercentage, address _owner ) external returns (PrizeVault) {
address(0) Check in ConstructorThe constructor does not include a check for address(0) when set state variables that hold addresses.
Initializing a state variable with address(0) can lead to unintended behavior and vulnerabilities in the contract, such as sending funds to an inaccessible address.
It is recommended to include a validation step to ensure that address parameters are not set to address(0).
File: pt-v5-vault/src/PrizeVault.sol /// @audit `yieldFeeRecipient_` has lack of `address(0)` check before use 289: constructor( string memory name_, string memory symbol_, IERC4626 yieldVault_, PrizePool prizePool_, address claimer_, address yieldFeeRecipient_, uint32 yieldFeePercentage_, uint256 yieldBuffer_, address owner_ ) TwabERC20(name_, symbol_, prizePool_.twabController()) Claimable(prizePool_, claimer_) Ownable(owner_) {
</details>File: pt-v5-vault/src/abstract/Claimable.sol /// @audit `claimer_` has lack of `address(0)` check before use 64: constructor(PrizePool prizePool_, address claimer_) {
Functions that call contracts or addresses with transfer hooks should use reentrancy guards for protection. Even if these functions adhere to the check-effects-interaction best practice, absence of reentrancy guards can expose the protocol users to read-only reentrancies. Without the guards, the only protective measure is to block-list the entire protocol, which isn't an optimal solution.
<details> <summary><i>4 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol /// @audit function `_tryGetAssetDecimals()` 773: (bool success, bytes memory encodedDecimals) = address(asset_).staticcall( abi.encodeWithSelector(IERC20Metadata.decimals.selector) ); /// @audit function `_depositAndMint()` 854: _asset.safeTransferFrom( _caller, address(this), _assets ); /// @audit function `_withdraw()` 939: _asset.transfer(_receiver, _assets);
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol /// @audit function `deployVault()` 118: IERC20(_vault.asset()).transferFrom(msg.sender, address(_vault), YIELD_BUFFER);
Ownable2Step rather than OwnableTo enhance the security and prevent inadvertent ownership transfers, it's advisable to use Ownable2Step or Ownable2StepUpgradeable.
Contracts necessitate an active confirmation from the recipient before the ownership transfer is finalized.
This mechanism serves as a safeguard against scenarios where, for instance, a typo in the address could lead to unintentional ownership changes.
By implementing a two-step confirmation process, contracts can better ensure the accurate and intentional transfer of ownership.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 8: import { Ownable } from "owner-manager-contracts/Ownable.sol"; 65: contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownable {
Division by large numbers may result in the result being zero, due to Solidity not supporting fractions. Consider requiring a minimum amount for the numerator to ensure that it is always larger than the denominator.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 675: _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut;
Floating pragmas may lead to unintended vulnerabilities due to different compiler versions. It is recommended to lock the Solidity version in pragma statements.
<details> <summary><i>5 issue instances in 5 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 2: pragma solidity ^0.8.24
File: pt-v5-vault/src/PrizeVaultFactory.sol 2: pragma solidity ^0.8.24
File: pt-v5-vault/src/TwabERC20.sol 2: pragma solidity ^0.8.24
File: pt-v5-vault/src/abstract/Claimable.sol 2: pragma solidity ^0.8.24
</details>File: pt-v5-vault/src/abstract/HookManager.sol 2: pragma solidity ^0.8.0
For improved readability and maintainability, it's suggested to limit arithmetic operations to 3 per expression. Excessive operations can convolute the code, making it more prone to errors and more difficult to debug or review. Splitting operations across multiple lines is often a good approach. Special care should be taken with division operations. The number of arithmetic operations per line ideally shouldn't exceed three.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 675: (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut
address/ID mappings can be combined into a single mapping of an address/ID to a struct, for readabilityCombining multiple address/ID mappings into a single mapping to a struct can enhance code clarity and maintainability. Consider refactoring multiple mappings into a single mapping with a struct for cleaner code structure. This arrangement also promotes a more organized contract structure, making it easier for developers to navigate and understand.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVaultFactory.sol 69: mapping(address vault => bool deployedByFactory) public deployedVaults 72: mapping(address deployer => uint256 nonce) public deployerNonces
struct rather than having many function input parametersFunctions with many parameters can become difficult to read and maintain. Using a struct to encapsulate these parameters can improve code readability, increase reusability, and reduce the likelihood of errors. Consider refactoring functions that take more than three parameters to use a struct instead.
<details> <summary><i>5 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 289: constructor( string memory name_, string memory symbol_, IERC4626 yieldVault_, PrizePool prizePool_, address claimer_, address yieldFeeRecipient_, uint32 yieldFeePercentage_, uint256 yieldBuffer_, address owner_ ) TwabERC20(name_, symbol_, prizePool_.twabController()) Claimable(prizePool_, claimer_) Ownable(owner_) 524: function depositWithPermit( uint256 _assets, address _owner, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s ) external returns (uint256) 887: function _burnAndWithdraw( address _caller, address _receiver, address _owner, uint256 _shares, uint256 _assets ) internal
File: pt-v5-vault/src/PrizeVaultFactory.sol 92: function deployVault( string memory _name, string memory _symbol, IERC4626 _yieldVault, PrizePool _prizePool, address _claimer, address _yieldFeeRecipient, uint32 _yieldFeePercentage, address _owner ) external returns (PrizeVault)
</details>File: pt-v5-vault/src/abstract/Claimable.sol 76: function claimPrize( address _winner, uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256)
constants when passing zero as a function argumentIn instances where utilizing a zero parameter is essential, it is recommended to employ descriptive constants or an enum instead of directly integrating zero within function calls. This strategy aids in clearly articulating the caller's intention and minimizes the risk of errors. Emitting zero also not recomended, as it is not clear what the intention is.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 869: _asset.approve(address(yieldVault), 0)
Take advantage of custom error's return value property.
An important feature of Custom Error is that values such as address, tokenID, msg.value can be written inside the () sign, providing a serious advantage in debugging and examining the revert details of dapps such as Tenderly.
<details> <summary><i>14 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 182: error YieldVaultZeroAddress() 185: error OwnerZeroAddress() 188: error WithdrawZeroAssets() 191: error BurnZeroShares() 194: error DepositZeroAssets() 197: error MintZeroShares() 200: error ZeroTotalAssets() 203: error LPZeroAddress() 206: error SweepZeroAssets() 209: error LiquidationAmountOutZero()
182 | 185 | 188 | 191 | 194 | 197 | 200 | 203 | 206 | 209
File: pt-v5-vault/src/TwabERC20.sol 33: error TwabControllerZeroAddress()
</details>File: pt-v5-vault/src/abstract/Claimable.sol 34: error PrizePoolZeroAddress() 37: error ClaimerZeroAddress() 40: error ClaimRecipientZeroAddress()
OpenZeppelin's has SafeCast library that provides functions to safely cast. Recommended to use it instead of casting directly in any case.
</details>File: pt-v5-vault/src/PrizeVault.sol 779: uint8(returnedDecimals)
This will allow users to easily exactly pinpoint when and by whom a contract was constructed.
<details> <summary><i>3 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 289: constructor( string memory name_, string memory symbol_, IERC4626 yieldVault_, PrizePool prizePool_, address claimer_, address yieldFeeRecipient_, uint32 yieldFeePercentage_, uint256 yieldBuffer_, address owner_ ) TwabERC20(name_, symbol_, prizePool_.twabController()) Claimable(prizePool_, claimer_) Ownable(owner_)
File: pt-v5-vault/src/TwabERC20.sol 42: constructor( string memory name_, string memory symbol_, TwabController twabController_ ) ERC20(name_, symbol_) ERC20Permit(name_)
</details>File: pt-v5-vault/src/abstract/Claimable.sol 64: constructor(PrizePool prizePool_, address claimer_)
While the compiler automatically provides a getter for accessing single elements within a public state variable array, it doesn't provide a way to fetch the whole array, or subsets thereof. Consider adding a function to allow the fetching of slices of the array, especially if the contract doesn't already have multicall functionality.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVaultFactory.sol 66: PrizeVault[] public allVaults
Using named returns makes the code more self-documenting, makes it easier to fill out NatSpec, and in some cases can save gas. The cases below are where there currently is at most one return statement, which is ideal for named returns.
<details> <summary><i>39 issue instances in 5 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 320: function decimals() public view override(ERC20, IERC20Metadata) returns (uint8) 329: function asset() external view returns (address) 336: function totalAssets() public view returns (uint256) 341: function convertToShares(uint256 _assets) public view returns (uint256) 355: function convertToAssets(uint256 _shares) public view returns (uint256) 374: function maxDeposit(address) public view returns (uint256) 397: function maxMint(address _owner) public view returns (uint256) 404: function maxWithdraw(address _owner) public view returns (uint256) 415: function maxRedeem(address _owner) public view returns (uint256) 441: function previewDeposit(uint256 _assets) public pure returns (uint256) 447: function previewMint(uint256 _shares) public pure returns (uint256) 454: function previewWithdraw(uint256 _assets) public view returns (uint256) 470: function previewRedeem(uint256 _shares) public view returns (uint256) 475: function deposit(uint256 _assets, address _receiver) external returns (uint256) 482: function mint(uint256 _shares, address _receiver) external returns (uint256) 489: function withdraw( uint256 _assets, address _receiver, address _owner ) external returns (uint256) 500: function redeem( uint256 _shares, address _receiver, address _owner ) external returns (uint256) 524: function depositWithPermit( uint256 _assets, address _owner, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s ) external returns (uint256) 552: function sponsor(uint256 _assets) external returns (uint256) 573: function totalDebt() public view returns (uint256) 584: function totalYieldBalance() public view returns (uint256) 591: function availableYieldBalance() public view returns (uint256) 597: function currentYieldBuffer() external view returns (uint256) 631: function liquidatableBalanceOf(address _tokenOut) public view returns (uint256) 659: function transferTokensOut( address, address _receiver, address _tokenOut, uint256 _amountOut ) public virtual onlyLiquidationPair returns (bytes memory) 717: function targetOf(address) external view returns (address) 722: function isLiquidationPair( address _tokenOut, address _liquidationPair ) external view returns (bool) 772: function _tryGetAssetDecimals(IERC20 asset_) internal view returns (bool, uint8) 790: function _totalDebt(uint256 _totalSupply) internal view returns (uint256) 798: function _twabSupplyLimit(uint256 _totalSupply) internal pure returns (uint256) 808: function _totalYieldBalance(uint256 _totalAssets, uint256 totalDebt_) internal pure returns (uint256) 823: function _availableYieldBalance(uint256 _totalAssets, uint256 totalDebt_) internal view returns (uint256) 921: function _maxYieldVaultWithdraw() internal view returns (uint256)
320 | 329 | 336 | 341 | 355 | 374 | 397 | 404 | 415 | 441 | 447 | 454 | 470 | 475 | 482 | 489 | 500 | 524 | 552 | 573 | 584 | 591 | 597 | 631 | 659 | 717 | 722 | 772 | 790 | 798 | 808 | 823 | 921
File: pt-v5-vault/src/PrizeVaultFactory.sol 92: function deployVault( string memory _name, string memory _symbol, IERC4626 _yieldVault, PrizePool _prizePool, address _claimer, address _yieldFeeRecipient, uint32 _yieldFeePercentage, address _owner ) external returns (PrizeVault) 136: function totalVaults() external view returns (uint256)
File: pt-v5-vault/src/TwabERC20.sol 56: function balanceOf( address _account ) public view virtual override(ERC20) returns (uint256) 63: function totalSupply() public view virtual override(ERC20) returns (uint256)
File: pt-v5-vault/src/abstract/Claimable.sol 76: function claimPrize( address _winner, uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256)
</details>File: pt-v5-vault/src/abstract/HookManager.sol 22: function getHooks(address account) external view returns (VaultHooks memory)
When calling functions in external contracts with multiple arguments, consider using named function parameters, rather than positional ones.
<details> <summary><i>4 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 540: IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s) 713: prizePool.contributePrizeTokens(address(this), _amountIn) 936: yieldVault.redeem(_yieldVaultShares, address(this), address(this))
</details>File: pt-v5-vault/src/abstract/Claimable.sol 99: prizePool.claimPrize( _winner, _tier, _prizeIndex, recipient, _reward, _rewardRecipient )
Events should include the sender information when emitted in public or external functions for better traceability.
File: pt-v5-vault/src/PrizeVault.sol /// @audit external function `sponsor()` emits an event without sender information 561: emit Sponsor(_owner, _assets, _shares); /// @audit external function `setLiquidationPair()` emits an event without sender information 746: emit LiquidationPairSet(address(this), address(_liquidationPair));
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol /// @audit external function `deployVault()` emits an event without sender information 122: emit NewPrizeVault( _vault, _yieldVault, _prizePool, _name, _symbol );
0.8.23The recent updates in Solidity provide several features and optimizations that, when leveraged appropriately, can significantly improve your contract's code clarity and maintainability. Key enhancements include the use of push0 for placing 0 on the stack for EVM versions starting from "Shanghai", making your code simpler and more straightforward. Moreover, Solidity has extended NatSpec documentation support to enum and struct definitions, facilitating more comprehensive and insightful code documentation.
Additionally, the re-implementation of the UnusedAssignEliminator and UnusedStoreEliminator in the Solidity optimizer provides the ability to remove unused assignments in deeply nested loops. This results in a cleaner, more efficient contract code, reducing clutter and potential points of confusion during code review or debugging. It's recommended to make full use of these features and optimizations to enhance the robustness and readability of your smart contracts.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/abstract/HookManager.sol 2: pragma solidity ^0.8.0;
@param tag is missingNatural Specification (NatSpec) comments are crucial for understanding the role of function arguments in your Solidity code.
Including @param tags will not only improve your code's readability but also its maintainability by clearly defining each argument's purpose.
More information in Documentation
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/TwabERC20.sol /// @audit Missed @param `twabController_` 42: constructor( string memory name_, string memory symbol_, TwabController twabController_ ) ERC20(name_, symbol_) ERC20Permit(name_) {
Functions with high cyclomatic complexity are harder to understand, test, and maintain. Consider breaking down these blocks into more manageable units, by splitting things into utility functions, by reducing nesting, and by using early returns.
Learn More About Cyclomatic Complexity
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol /// @audit function `transferTokensOut` has a cyclomatic complexity of 7 659: function transferTokensOut( address, address _receiver, address _tokenOut, uint256 _amountOut ) public virtual onlyLiquidationPair returns (bytes memory) {
require()/revert() as well as custom errorsThe contract uses both require()/revert() and custom errors for error handling.
It's recommended to use a consistent approach for error handling in a single file.
<details> <summary><i>2 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 65: contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownable {
</details>File: pt-v5-vault/src/TwabERC20.sol 19: contract TwabERC20 is ERC20, ERC20Permit {
The current Solidity version used in the code is too recent to be trusted. Using a newer version might introduce unrecovered bugs. It is recommended to use version 0.8.21.
<details> <summary><i>5 issue instances in 5 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 2: pragma solidity ^0.8.24;
File: pt-v5-vault/src/PrizeVaultFactory.sol 2: pragma solidity ^0.8.24;
File: pt-v5-vault/src/TwabERC20.sol 2: pragma solidity ^0.8.24;
File: pt-v5-vault/src/abstract/Claimable.sol 2: pragma solidity ^0.8.24;
</details>File: pt-v5-vault/src/interfaces/IVaultHooks.sol 2: pragma solidity ^0.8.24;
Some lines use // x and some use //x. The instances below point out the usages that don't follow the majority, within each file:
<details> <summary><i>140 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVaultFactory.sol 10: /// @title PoolTogether V5 Prize Vault Factory 11: /// @author PoolTogether Inc. & G9 Software Inc. 12: /// @notice Factory contract for deploying new prize vaults using a standard underlying ERC4626 yield vault. 15: //////////////////////////////////////////////////////////////////////////////// 17: //////////////////////////////////////////////////////////////////////////////// 19: /// @notice Emitted when a new PrizeVault has been deployed by this factory. 20: /// @param vault The vault that was deployed 21: /// @param yieldVault The underlying yield vault 22: /// @param prizePool The prize pool the vault contributes to 23: /// @param name The name of the vault token 24: /// @param symbol The symbol for the vault token 33: //////////////////////////////////////////////////////////////////////////////// 35: //////////////////////////////////////////////////////////////////////////////// 37: /// @notice The yield buffer to use for vault deployments. 38: /// @dev The yield buffer is expected to be of insignificant value and is used to cover rounding 39: /// errors on deposits and withdrawals. Yield is expected to accrue faster than the yield buffer 40: /// can be reasonably depleted. 41: /// 42: /// The yield buffer should be set as high as possible while still being considered 43: /// insignificant for the lowest precision per dollar asset that is expected to be supported. 44: /// 45: /// Precision per dollar (PPD) can be calculated by: (10 ^ DECIMALS) / ($ value of 1 asset). 46: /// For example, USDC has a PPD of (10 ^ 6) / ($1) = 10e6 p/$. 47: /// 48: /// As a rule of thumb, assets with lower PPD than USDC should not be assumed to be compatible since 49: /// the potential loss of a single unit rounding error is likely too high to be made up by yield at 50: /// a reasonable rate. Actual results may vary based on expected gas costs, asset fluctuation, and 51: /// yield accrual rates. 52: /// 53: /// The yield buffer of vaults deployed by this factory is 1e5. This means that if you deploy a 54: /// vault with USDC as the underlying asset, you will have to approve this factory to spend 1e5 55: /// USDC ($0.10) to be sent to the prize vault during deployment. This value will cover the first 56: /// 100k rounding errors on deposits and withdraws to the vault and is not recoverable by the 57: /// deployer. 58: /// 59: /// If the yield buffer is depleted on a vault, the vault will prevent any further 60: /// deposits if it would result in a rounding error and any rounding errors incurred by withdrawals 61: /// will not be covered by yield. The yield buffer will be replenished automatically as yield accrues 62: /// on deposits. 65: /// @notice List of all vaults deployed by this factory. 68: /// @notice Mapping to verify if a Vault has been deployed via this factory. 71: /// @notice Mapping to store deployer nonces for CREATE2 74: //////////////////////////////////////////////////////////////////////////////// 76: //////////////////////////////////////////////////////////////////////////////// 78: /// @notice Deploy a new vault 79: /// @dev Emits a `NewPrizeVault` event with the vault details. 80: /// @dev `claimer` can be set to address zero if none is available yet. 81: /// @dev The caller MUST approve this factory to spend underlying assets equal to `YIELD_BUFFER` so the yield 82: /// buffer can be filled on deployment. This value is unrecoverable and is expected to be insignificant. 83: /// @param _name Name of the ERC20 share minted by the vault 84: /// @param _symbol Symbol of the ERC20 share minted by the vault 85: /// @param _yieldVault Address of the ERC4626 vault in which assets are deposited to generate yield 86: /// @param _prizePool Address of the PrizePool that computes prizes 87: /// @param _claimer Address of the claimer 88: /// @param _yieldFeeRecipient Address of the yield fee recipient 89: /// @param _yieldFeePercentage Yield fee percentage 90: /// @param _owner Address that will gain ownership of this contract 91: /// @return PrizeVault The newly deployed PrizeVault 134: /// @notice Total number of vaults deployed by this factory. 135: /// @return uint256 Number of vaults deployed by this factory.
10 | 11 | 12 | 15 | 17 | 19 | 20 | 21 | 22 | 23 | 24 | 33 | 35 | 37 | 38 | 39 | 40 | 41 | 42 | 43 | 44 | 45 | 46 | 47 | 48 | 49 | 50 | 51 | 52 | 53 | 54 | 55 | 56 | 57 | 58 | 59 | 60 | 61 | 62 | 65 | 68 | 71 | 74 | 76 | 78 | 79 | 80 | 81 | 82 | 83 | 84 | 85 | 86 | 87 | 88 | 89 | 90 | 91 | 134 | 135
File: pt-v5-vault/src/TwabERC20.sol 10: /// @title PoolTogether V5 TWAB ERC20 Token 11: /// @author G9 Software Inc. 12: /// @notice This contract creates an ERC20 token with balances stored in a TwabController, 13: /// enabling time-weighted average balances for each depositor and token compatibility 14: /// with the PoolTogether V5 Prize Pool. 15: /// @dev This contract is designed to be used as an accounting layer when building a vault 16: /// for PoolTogether V5. 17: /// @dev The TwabController limits all balances including total token supply to uint96 for 18: /// gas savings. Any mints that increase a balance past this limit will fail. 21: //////////////////////////////////////////////////////////////////////////////// 23: //////////////////////////////////////////////////////////////////////////////// 25: /// @notice Address of the TwabController used to keep track of balances. 28: //////////////////////////////////////////////////////////////////////////////// 30: //////////////////////////////////////////////////////////////////////////////// 32: /// @notice Thrown if the TwabController address is the zero address. 35: //////////////////////////////////////////////////////////////////////////////// 37: //////////////////////////////////////////////////////////////////////////////// 39: /// @notice TwabERC20 Constructor 40: /// @param name_ The name of the token 41: /// @param symbol_ The token symbol 51: //////////////////////////////////////////////////////////////////////////////// 53: //////////////////////////////////////////////////////////////////////////////// 55: /// @inheritdoc ERC20 62: /// @inheritdoc ERC20 67: //////////////////////////////////////////////////////////////////////////////// 69: //////////////////////////////////////////////////////////////////////////////// 71: /// @notice Mints tokens to `_receiver` and increases the total supply. 72: /// @dev Emits a {Transfer} event with `from` set to the zero address. 73: /// @dev `_receiver` cannot be the zero address. 74: /// @param _receiver Address that will receive the minted tokens 75: /// @param _amount Tokens to mint 81: /// @notice Destroys tokens from `_owner` and reduces the total supply. 82: /// @dev Emits a {Transfer} event with `to` set to the zero address. 83: /// @dev `_owner` cannot be the zero address. 84: /// @dev `_owner` must have at least `_amount` tokens. 85: /// @param _owner The owner of the tokens 86: /// @param _amount The amount of tokens to burn 92: /// @notice Transfers tokens from one account to another. 93: /// @dev Emits a {Transfer} event. 94: /// @dev `_from` cannot be the zero address. 95: /// @dev `_to` cannot be the zero address. 96: /// @dev `_from` must have a balance of at least `_amount`. 97: /// @param _from Address to transfer from 98: /// @param _to Address to transfer to 99: /// @param _amount The amount of tokens to transfer
10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 21 | 23 | 25 | 28 | 30 | 32 | 35 | 37 | 39 | 40 | 41 | 51 | 53 | 55 | 62 | 67 | 69 | 71 | 72 | 73 | 74 | 75 | 81 | 82 | 83 | 84 | 85 | 86 | 92 | 93 | 94 | 95 | 96 | 97 | 98 | 99
File: pt-v5-vault/src/abstract/Claimable.sol 9: /// @title PoolTogether V5 Claimable Vault Extension 10: /// @author G9 Software Inc. 11: /// @notice Provides an interface for Claimer contracts to interact with a vault in PoolTogether 12: /// V5 while allowing each account to set and manage prize hooks that are called when they win. 15: //////////////////////////////////////////////////////////////////////////////// 17: //////////////////////////////////////////////////////////////////////////////// 19: /// @notice The gas to give to each of the before and after prize claim hooks. 20: /// @dev This should be enough gas to mint an NFT if needed. 23: /// @notice Address of the PrizePool that computes prizes. 26: /// @notice Address of the claimer. 29: //////////////////////////////////////////////////////////////////////////////// 31: //////////////////////////////////////////////////////////////////////////////// 33: /// @notice Thrown when the Prize Pool is set to the zero address. 36: /// @notice Thrown when the Claimer is set to the zero address. 39: /// @notice Thrown when a prize is claimed for the zero address. 42: /// @notice Thrown when the caller is not the prize claimer. 43: /// @param caller The caller address 44: /// @param claimer The claimer address 47: //////////////////////////////////////////////////////////////////////////////// 49: //////////////////////////////////////////////////////////////////////////////// 51: /// @notice Requires the caller to be the claimer. 57: //////////////////////////////////////////////////////////////////////////////// 59: //////////////////////////////////////////////////////////////////////////////// 61: /// @notice Claimable constructor 62: /// @param prizePool_ The prize pool to claim prizes from 63: /// @param claimer_ The address allowed to claim prizes on behalf of winners 70: //////////////////////////////////////////////////////////////////////////////// 72: //////////////////////////////////////////////////////////////////////////////// 74: /// @inheritdoc IClaimable 75: /// @dev Also calls the before and after claim hooks if set by the winner. 121: //////////////////////////////////////////////////////////////////////////////// 123: //////////////////////////////////////////////////////////////////////////////// 125: /// @notice Set claimer address. 126: /// @dev Will revert if `_claimer` is address zero. 127: /// @param _claimer Address of the claimer
9 | 10 | 11 | 12 | 15 | 17 | 19 | 20 | 23 | 26 | 29 | 31 | 33 | 36 | 39 | 42 | 43 | 44 | 47 | 49 | 51 | 57 | 59 | 61 | 62 | 63 | 70 | 72 | 74 | 75 | 121 | 123 | 125 | 126 | 127
</details>error definitionCustom errors that aren't utilized within the contract add unnecessary complexity to the codebase. Cleaning up and removing these unused custom errors can enhance the readability and maintainability of the contract.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol /// @audit Custom Error `SweepZeroAssets` declared but never used 206: error SweepZeroAssets();
It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).
<details> <summary><i>4 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 735: function setClaimer(address _claimer) external onlyOwner { 742: function setLiquidationPair(address _liquidationPair) external onlyOwner { 753: function setYieldFeePercentage(uint32 _yieldFeePercentage) external onlyOwner { 759: function setYieldFeeRecipient(address _yieldFeeRecipient) external onlyOwner {
The current Solidity version used in the contract is outdated. Consider using a more recent version for improved features and security.
0.8.4: bytes.concat() instead of abi.encodePacked(,)
0.8.12: string.concat() instead of abi.encodePacked(,)
0.8.13: Ability to use using for with a list of free functions
0.8.14: ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases. Override Checker: Allow changing data location for parameters only when overriding external functions.
0.8.15: Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays. Yul Optimizer: Keep all memory side-effects of inline assembly blocks.
0.8.16: Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.
0.8.17: Yul Optimizer: Prevent the incorrect removal of storage writes before calls to Yul functions that conditionally terminate the external EVM call.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/abstract/HookManager.sol 2: pragma solidity ^0.8.0;
To avoid mistakenly accepting empty bytes as valid parameters, it is advisable to implement checks for non-empty bytes within functions. This ensures that empty bytes are not treated as valid inputs, preventing potential issues.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 524: function depositWithPermit( uint256 _assets, address _owner, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s ) external returns (uint256) { 524: function depositWithPermit( uint256 _assets, address _owner, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s ) external returns (uint256) {
Underscore before of after function argument names is a common convention in Solidity NOT a documentation requirement.
Function arguments should use mixedCase for better readability and consistency with Solidity style guidelines. Examples of good practice include: initialSupply, account, recipientAddress, senderAddress, newOwner. More information in Documentation
Rule exceptions
_ at the beginning of the mixedCase match for private variables and unused parameters.File: pt-v5-vault/src/PrizeVault.sol 341: function convertToShares(uint256 _assets) public view returns (uint256) { 355: function convertToAssets(uint256 _shares) public view returns (uint256) { 397: function maxMint(address _owner) public view returns (uint256) { 404: function maxWithdraw(address _owner) public view returns (uint256) { 415: function maxRedeem(address _owner) public view returns (uint256) { 441: function previewDeposit(uint256 _assets) public pure returns (uint256) { 447: function previewMint(uint256 _shares) public pure returns (uint256) { 454: function previewWithdraw(uint256 _assets) public view returns (uint256) { 470: function previewRedeem(uint256 _shares) public view returns (uint256) { 475: function deposit(uint256 _assets, address _receiver) external returns (uint256) { 482: function mint(uint256 _shares, address _receiver) external returns (uint256) { 489: function withdraw( uint256 _assets, address _receiver, address _owner ) external returns (uint256) { 500: function redeem( uint256 _shares, address _receiver, address _owner ) external returns (uint256) { 524: function depositWithPermit( uint256 _assets, address _owner, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s ) external returns (uint256) { 552: function sponsor(uint256 _assets) external returns (uint256) { 611: function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient { 631: function liquidatableBalanceOf(address _tokenOut) public view returns (uint256) { 659: function transferTokensOut( address, address _receiver, address _tokenOut, uint256 _amountOut ) public virtual onlyLiquidationPair returns (bytes memory) { 703: function verifyTokensIn( address _tokenIn, uint256 _amountIn, bytes calldata ) external onlyLiquidationPair { 722: function isLiquidationPair( address _tokenOut, address _liquidationPair ) external view returns (bool) { 735: function setClaimer(address _claimer) external onlyOwner { 742: function setLiquidationPair(address _liquidationPair) external onlyOwner { 753: function setYieldFeePercentage(uint32 _yieldFeePercentage) external onlyOwner { 759: function setYieldFeeRecipient(address _yieldFeeRecipient) external onlyOwner { 772: function _tryGetAssetDecimals(IERC20 asset_) internal view returns (bool, uint8) { 790: function _totalDebt(uint256 _totalSupply) internal view returns (uint256) { 798: function _twabSupplyLimit(uint256 _totalSupply) internal pure returns (uint256) { 808: function _totalYieldBalance(uint256 _totalAssets, uint256 totalDebt_) internal pure returns (uint256) { 823: function _availableYieldBalance(uint256 _totalAssets, uint256 totalDebt_) internal view returns (uint256) { 843: function _depositAndMint(address _caller, address _receiver, uint256 _assets, uint256 _shares) internal { 887: function _burnAndWithdraw( address _caller, address _receiver, address _owner, uint256 _shares, uint256 _assets ) internal { 928: function _withdraw(address _receiver, uint256 _assets) internal { 947: function _setYieldFeePercentage(uint32 _yieldFeePercentage) internal { 958: function _setYieldFeeRecipient(address _yieldFeeRecipient) internal { 289: constructor( string memory name_, string memory symbol_, IERC4626 yieldVault_, PrizePool prizePool_, address claimer_, address yieldFeeRecipient_, uint32 yieldFeePercentage_, uint256 yieldBuffer_, address owner_ ) TwabERC20(name_, symbol_, prizePool_.twabController()) Claimable(prizePool_, claimer_) Ownable(owner_) {
341 | 355 | 397 | 404 | 415 | 441 | 447 | 454 | 470 | 475 | 482 | 489 | 500 | 524 | 552 | 611 | 631 | 659 | 703 | 722 | 735 | 742 | 753 | 759 | 772 | 790 | 798 | 808 | 823 | 843 | 887 | 928 | 947 | 958 | 289
File: pt-v5-vault/src/PrizeVaultFactory.sol 92: function deployVault( string memory _name, string memory _symbol, IERC4626 _yieldVault, PrizePool _prizePool, address _claimer, address _yieldFeeRecipient, uint32 _yieldFeePercentage, address _owner ) external returns (PrizeVault) {
File: pt-v5-vault/src/TwabERC20.sol 56: function balanceOf( address _account ) public view virtual override(ERC20) returns (uint256) { 76: function _mint(address _receiver, uint256 _amount) internal virtual override { 87: function _burn(address _owner, uint256 _amount) internal virtual override { 100: function _transfer(address _from, address _to, uint256 _amount) internal virtual override { 42: constructor( string memory name_, string memory symbol_, TwabController twabController_ ) ERC20(name_, symbol_) ERC20Permit(name_) {
</details>File: pt-v5-vault/src/abstract/Claimable.sol 76: function claimPrize( address _winner, uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256) { 128: function _setClaimer(address _claimer) internal { 64: constructor(PrizePool prizePool_, address claimer_) {
openzeppelin to the Latest Version - 5.0.0These contracts import contracts from @openzeppelin/contracts but they are not using the latest version. For more information, please visit: OpenZeppelin GitHub Releases It is recommended to always use the latest version to take advantage of updates and security fixes.
<details> <summary><i>8 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 4: import { IERC4626 } from "openzeppelin/interfaces/IERC4626.sol"; 5: import { SafeERC20, IERC20Permit } from "openzeppelin/token/ERC20/utils/SafeERC20.sol"; 6: import { ERC20, IERC20, IERC20Metadata } from "openzeppelin/token/ERC20/ERC20.sol"; 7: import { Math } from "openzeppelin/utils/math/Math.sol";
File: pt-v5-vault/src/PrizeVaultFactory.sol 4: import { IERC20, IERC4626 } from "openzeppelin/token/ERC20/extensions/ERC4626.sol";
</details>File: pt-v5-vault/src/TwabERC20.sol 4: import { ERC20 } from "openzeppelin/token/ERC20/ERC20.sol"; 5: import { ERC20Permit } from "openzeppelin/token/ERC20/extensions/ERC20Permit.sol"; 6: import { SafeCast } from "openzeppelin/utils/math/SafeCast.sol";
require()/revert() checks should be refactored to a modifier or functionDuplicated require() or revert() checks should be refactored to a modifier or function. This helps in maintaining a clean and organized codebase and saves deployment costs.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 612: if (_shares == 0) revert MintZeroShares(); 844: if (_shares == 0) revert MintZeroShares();
indexedIndex event fields make the field more quickly accessible to off-chain tools that parse events. This is especially useful when it comes to filtering based on an address. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Where applicable, each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three applicable fields, all of the applicable fields should be indexed.
<details> <summary><i>4 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 150: event YieldFeePercentageSet(uint256 yieldFeePercentage); 156: event Sponsor(address indexed caller, uint256 assets, uint256 shares); 175: event ClaimYieldFeeShares(address indexed recipient, uint256 shares);
</details>File: pt-v5-vault/src/abstract/HookManager.sol 14: event SetHooks(address indexed account, VaultHooks hooks);
safePermit() Instead of permit()The use of the permit() function can be risky as observed in the Anyswap hack where the function didn't really exist, but the fallback function didn't revert the transaction. In particular, tokens like WETH, BNB, and HEX exhibit a 'phantom permit'—they accept any call to permit() without reverting, posing a security risk.
To mitigate this risk, consider using safePermit() which ensures that the permit function call actually goes through as intended.
For more details, see Phantom Functions and the Billion Dollar No-op.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 540: IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s);
Smart contracts that hold significant value, interact with external contracts, or have complex logic should include an emergency-stop mechanism for added security. This allows pausing certain contract functionalities in case of emergencies, mitigating potential damages.
This contract seems to lack such a mechanism. Implementing an emergency stop can enhance contract security and reliability.
<details> <summary><i>2 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 1: No emergency stop pattern found
</details>File: pt-v5-vault/src/TwabERC20.sol 1: No emergency stop pattern found
While adding a block or deny-list may increase the level of centralization in the contract, it provides an additional layer of security by preventing hackers from using stolen tokens or carrying out other malicious activities.
Although it's a trade-off, a block or deny-list can help improve the overall security posture of the contract.
<details> <summary><i>2 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 65: contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownable {
</details>File: pt-v5-vault/src/TwabERC20.sol 19: contract TwabERC20 is ERC20, ERC20Permit {
immutable VariablesFor better readability and adherence to common naming conventions, variable names declared as immutable should be written in all uppercase letters.
<details> <summary><i>6 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 112: uint256 public immutable yieldBuffer; 115: IERC4626 public immutable yieldVault; 135: IERC20 private immutable _asset; 138: uint8 private immutable _underlyingDecimals;
File: pt-v5-vault/src/TwabERC20.sol 26: TwabController public immutable twabController;
</details>File: pt-v5-vault/src/abstract/Claimable.sol 24: PrizePool public immutable prizePool;
Complex functions spanning multiple lines should have more comments to better explain the purpose of each logic step. Proper commenting can greatly improve the readability and maintainability of the codebase. Consider adding explicit comments to provide insights into the function's operation.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/abstract/Claimable.sol /// @audit function with 31 lines has only 0 comment lines 76: function claimPrize( address _winner, uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256) {
constants should be defined rather than using magic numbersMagic numbers are hardcoded numerical values used directly in the code, making it harder to read and maintain. Consider using constants instead of magic numbers.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol /// @audit 18 305: _underlyingDecimals = success ? assetDecimals : 18; /// @audit 32 776: if (success && encodedDecimals.length >= 32) {
msg.sender checks to a common authorization modifierFunctions that are only allowed to be called by a specific actor should use a modifier to check if the caller is the specified actor (e.g., owner, specific role, etc.).
Using require to check msg.sender in the function body is less efficient and less clear.
Consider refactoring these require statements into a modifier for better readability, organization, and gas efficiency.
</details>File: pt-v5-vault/src/PrizeVault.sol 532: if (_owner != msg.sender) {
@author tagIn the world of decentralized code, giving credit is key. NatSpec's @author tag acknowledges the minds behind the code.
It appears this Solidity contract omits the @author directive in its NatSpec annotations.
Properly attributing code to its contributors not only recognizes effort but also aids in establishing trust and credibility.
Dive Deeper into NatSpec Guidelines
</details>File: pt-v5-vault/src/PrizeVault.sol 65: contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownable {
System-wide constants should be declared in a single file for better maintainability and readability. This contract seems to contain constants which could potentially be system-wide and could be better managed if they were centralized in a single location.
<details> <summary><i>4 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 74: uint32 public constant FEE_PRECISION = 1e9; 80: uint32 public constant MAX_YIELD_FEE = 9e8;
File: pt-v5-vault/src/PrizeVaultFactory.sol 63: uint256 public constant YIELD_BUFFER = 1e5;
</details>File: pt-v5-vault/src/abstract/Claimable.sol 21: uint24 public constant HOOK_GAS = 150_000;
Having unnamed variables in function definitions can be confusing and decrease the code readability. Adding inline comments to explain the purpose of unnamed variables could make the code easier to understand and maintain.
For example, convert function declarations like function foo(address x, address) to function foo(address x, address /* y */) to indicate that the unnamed variable could have been named 'y'.
</details>File: pt-v5-vault/src/PrizeVault.sol /// @audit - `address` 374: function maxDeposit(address) public view returns (uint256) { /// @audit - `address` 659: function transferTokensOut( address, address _receiver, address _tokenOut, uint256 _amountOut ) public virtual onlyLiquidationPair returns (bytes memory) { /// @audit - `bytes` 703: function verifyTokensIn( address _tokenIn, uint256 _amountIn, bytes calldata ) external onlyLiquidationPair { /// @audit - `address` 717: function targetOf(address) external view returns (address) {
UpgradeableContract uses a non-upgradeable design. Transitioning to an upgradeable contract structure is more aligned with contemporary smart contract practices. This approach not only enhances flexibility but also allows for continuous improvement and adaptation, ensuring the contract stays relevant and robust in an ever-evolving ecosystem.
<details> <summary><i>5 issue instances in 5 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol /// @audit - Contract `PrizeVault` is not upgradeable 65: contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownable {
File: pt-v5-vault/src/PrizeVaultFactory.sol /// @audit - Contract `PrizeVaultFactory` is not upgradeable 13: contract PrizeVaultFactory {
File: pt-v5-vault/src/TwabERC20.sol /// @audit - Contract `TwabERC20` is not upgradeable 19: contract TwabERC20 is ERC20, ERC20Permit {
File: pt-v5-vault/src/abstract/Claimable.sol /// @audit - Contract `Claimable` is not upgradeable 13: abstract contract Claimable is HookManager, IClaimable {
</details>File: pt-v5-vault/src/abstract/HookManager.sol /// @audit - Contract `HookManager` is not upgradeable 9: abstract contract HookManager {
When emitting events that signify critical changes in parameters, it's important to include both the old and new values.
This aids in auditability, providing more complete information about the state change. It's especially necessary when the new value isn't required to be different from the old one, as this prevents ambiguity in event logs.
<details> <summary><i>2 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 952: emit YieldFeePercentageSet(_yieldFeePercentage);
</details>File: pt-v5-vault/src/abstract/Claimable.sol 131: emit ClaimerSet(_claimer);
Adhering to a recommended order in Solidity contracts enhances code readability and maintenance. More information in Documentation It's recommended to use the following order:
File: pt-v5-vault/src/PrizeVaultFactory.sol /// @audit `state variable` declared after `event` 63: uint256 public constant YIELD_BUFFER = 1e5;
</details>File: pt-v5-vault/src/abstract/HookManager.sol /// @audit `state variable` declared after `event` 17: mapping(address => VaultHooks) internal _hooks;
Following best practices for control structures in solidity code is vital for readability and maintainability. The control structures in contracts, libraries, functions, and structs should adhere to the following standards:
It is advised to revisit the control structures sections in documentation to ensure conformity with these best practices, fostering cleaner and more maintainable code.
<details> <summary><i>18 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol /// @audit `Return or revert statement should be on new line.` 300: if (address(yieldVault_) == address(0)) revert YieldVaultZeroAddress(); /// @audit `Return or revert statement should be on new line.` 301: if (owner_ == address(0)) revert OwnerZeroAddress(); /// @audit `Return or revert statement should be on new line.` 377: if (totalAssets() < totalDebt_) return 0; /// @audit `Return or revert statement should be on new line.` 458: if (_totalAssets == 0) revert ZeroTotalAssets(); /// @audit `Return or revert statement should be on new line.` 612: if (_shares == 0) revert MintZeroShares(); /// @audit `Return or revert statement should be on new line.` 615: if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance); /// @audit `Return or revert statement should be on new line.` 665: if (_amountOut == 0) revert LiquidationAmountOutZero(); /// @audit `Return or revert statement should be on new line.` 743: if (address(_liquidationPair) == address(0)) revert LPZeroAddress(); /// @audit `Return or revert statement should be on new line.` 844: if (_shares == 0) revert MintZeroShares(); /// @audit `Return or revert statement should be on new line.` 845: if (_assets == 0) revert DepositZeroAssets(); /// @audit `Return or revert statement should be on new line.` 873: if (totalAssets() < totalDebt()) revert LossyDeposit(totalAssets(), totalDebt()); /// @audit `Return or revert statement should be on new line.` 894: if (_assets == 0) revert WithdrawZeroAssets(); /// @audit `Return or revert statement should be on new line.` 895: if (_shares == 0) revert BurnZeroShares();
300 | 301 | 377 | 458 | 612 | 615 | 665 | 743 | 844 | 845 | 873 | 894 | 895
File: pt-v5-vault/src/TwabERC20.sol /// @audit `Return or revert statement should be on new line.` 47: if (address(0) == address(twabController_)) revert TwabControllerZeroAddress();
</details>File: pt-v5-vault/src/abstract/Claimable.sol /// @audit `Return or revert statement should be on new line.` 53: if (msg.sender != claimer) revert CallerNotClaimer(msg.sender, claimer); /// @audit `Return or revert statement should be on new line.` 65: if (address(prizePool_) == address(0)) revert PrizePoolZeroAddress(); /// @audit `Return or revert statement should be on new line.` 95: } if (recipient == address(0)) revert ClaimRecipientZeroAddress(); /// @audit `Return or revert statement should be on new line.` 129: if (_claimer == address(0)) revert ClaimerZeroAddress();
Placing constants on the left side of comparison statements can help prevent accidental assignments and improve code readability. In languages like C, placing constants on the left can protect against unintended assignments that would be treated as true conditions, leading to bugs. Although Solidity does not have this specific issue, using the same practice can still be beneficial for code readability and consistency.
Consider placing constants on the left side of comparison operators like ==, !=, <, >, <=, and >=."
File: pt-v5-vault/src/PrizeVault.sol 458: if (_totalAssets == 0) revert ZeroTotalAssets(); 612: if (_shares == 0) revert MintZeroShares(); 665: if (_amountOut == 0) revert LiquidationAmountOutZero(); 672: if (_yieldFeePercentage != 0) { 684: if (_yieldFee > 0) { 776: if (success && encodedDecimals.length >= 32) { 844: if (_shares == 0) revert MintZeroShares(); 845: if (_assets == 0) revert DepositZeroAssets(); 894: if (_assets == 0) revert WithdrawZeroAssets(); 895: if (_shares == 0) revert BurnZeroShares();
458 | 612 | 665 | 672 | 684 | 776 | 844 | 845 | 894 | 895
</details>@notice tagThe @notice tag in NatSpec annotations serves to provide information about the contract's functionality that is visible to end-users.
Including @notice comments helps to improve the contract's transparency and ease of understanding, contributing to the overall trust and credibility of the code.
NatSpec Guidelines
</details>File: pt-v5-vault/src/PrizeVault.sol 65: contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownable {
Functions that span many lines can be hard to understand and maintain. It is often beneficial to refactor long functions into multiple smaller functions. This improves readability, makes testing easier, and can even lead to gas optimization if it eliminates the need for variables that would otherwise have to be stored in memory.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/abstract/Claimable.sol /// @audit 31 lines (excluding comments) 76: function claimPrize( address _winner, uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256) {
@dev tagsNatSpec comments are a critical part of Solidity's documentation system, designed to help developers and others understand the behavior and purpose of a contract.
The @dev tag, in particular, provides context and insight into the contract's development considerations.
A missing @dev comment can lead to misunderstandings about the contract, making it harder for others to contribute to or use the contract effectively.
Therefore, it's highly recommended to include @dev comments in the documentation to enhance code readability and maintainability.
Refer to NatSpec Documentation
File: pt-v5-vault/src/PrizeVaultFactory.sol 13: contract PrizeVaultFactory {
</details>File: pt-v5-vault/src/interfaces/IVaultHooks.sol 17: interface IVaultHooks {
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
If a public function is not called internally within the contract, it should be declared as external to save gas.
File: pt-v5-vault/src/PrizeVault.sol 320: function decimals() public view override(ERC20, IERC20Metadata) returns (uint8) { 341: function convertToShares(uint256 _assets) public view returns (uint256) { 397: function maxMint(address _owner) public view returns (uint256) { 404: function maxWithdraw(address _owner) public view returns (uint256) { 584: function totalYieldBalance() public view returns (uint256) { 631: function liquidatableBalanceOf(address _tokenOut) public view returns (uint256) {
320 | 341 | 397 | 404 | 584 | 631
</details>else-block not requiredIn Solidity, if an if block contains a return statement, subsequent else blocks become unnecessary.
This is because the return statement halts the execution of the function at that point, making any else conditions redundant.
Therefore, omitting else after such if blocks can lead to cleaner and more readable code without any change in the functionality.
File: pt-v5-vault/src/PrizeVault.sol 344: if (_totalAssets >= totalDebt_) { return _assets; } else { // If the vault controls less assets than what has been deposited a share will be worth a // proportional amount of the total assets. This can happen due to fees, slippage, or loss // of funds in the underlying yield vault. return _assets.mulDiv(totalDebt_, _totalAssets, Math.Rounding.Down); } 358: if (_totalAssets >= totalDebt_) { return _shares; } else { // If the vault controls less assets than what has been deposited a share will be worth a // proportional amount of the total assets. This can happen due to fees, slippage, or loss // of funds in the underlying yield vault. return _shares.mulDiv(_totalAssets, totalDebt_, Math.Rounding.Down); } 384: if (_latentBalance >= _maxYieldVaultDeposit) { return 0; } else { unchecked { _maxDeposit = _maxYieldVaultDeposit - _latentBalance; } 422: if (_ownerShares > _maxWithdraw) { uint256 _totalAssets = totalAssets(); uint256 totalDebt_ = totalDebt(); if (_totalAssets >= totalDebt_) { return _maxWithdraw; } else { // Convert to shares while rounding up. Since 1 asset is guaranteed to be worth more than // 1 share and any upwards rounding will not exceed 1 share, we can be sure that when the // shares are converted back to assets (rounding down) the resulting asset value won't // exceed `_maxWithdraw`. uint256 _maxScaledRedeem = _maxWithdraw.mulDiv(totalDebt_, _totalAssets, Math.Rounding.Up); return _maxScaledRedeem >= _ownerShares ? _ownerShares : _maxScaledRedeem; } 461: if (_totalAssets >= totalDebt_) { return _assets; } else { // Follows the inverse conversion of `convertToAssets` return _assets.mulDiv(totalDebt_, _totalAssets, Math.Rounding.Up); } 600: if (totalYieldBalance_ >= _yieldBuffer) { return _yieldBuffer; } else { return totalYieldBalance_; } 809: if (totalDebt_ >= _totalAssets) { return 0; } else { unchecked { return _totalAssets - totalDebt_; }
344 | 358 | 384 | 422 | 461 | 600 | 809
</details>Ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier. But there are contracts in the project that do not comply with this. Functions should be grouped according to their visibility and ordered:
File: pt-v5-vault/src/PrizeVault.sol /// @audit `public` function `decimals()` declared before `external` function `asset()` 320: function decimals() public view override(ERC20, IERC20Metadata) returns (uint8) { 329: function asset() external view returns (address) { /// @audit `public` function `previewRedeem()` declared before `external` function `deposit()` 470: function previewRedeem(uint256 _shares) public view returns (uint256) { 475: function deposit(uint256 _assets, address _receiver) external returns (uint256) { /// @audit `public` function `availableYieldBalance()` declared before `external` function `currentYieldBuffer()` 591: function availableYieldBalance() public view returns (uint256) { 597: function currentYieldBuffer() external view returns (uint256) { /// @audit `public` function `transferTokensOut()` declared before `external` function `verifyTokensIn()` 659: function transferTokensOut( address, address _receiver, address _tokenOut, uint256 _amountOut ) public virtual onlyLiquidationPair returns (bytes memory) { 703: function verifyTokensIn( address _tokenIn, uint256 _amountIn, bytes calldata ) external onlyLiquidationPair {
320 | 329 | 470 | 475 | 591 | 597 | 659 | 703
</details>See the whitespace section of the Solidity Style Guide
<details> <summary><i>47 issue instances in 5 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { IERC4626 } from "openzeppelin/interfaces/IERC4626.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 5: import { SafeERC20, IERC20Permit } from "openzeppelin/token/ERC20/utils/SafeERC20.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 6: import { ERC20, IERC20, IERC20Metadata } from "openzeppelin/token/ERC20/ERC20.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 7: import { Math } from "openzeppelin/utils/math/Math.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 8: import { Ownable } from "owner-manager-contracts/Ownable.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 10: import { Claimable } from "./abstract/Claimable.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 11: import { TwabERC20 } from "./TwabERC20.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 13: import { ILiquidationSource } from "pt-v5-liquidator-interfaces/ILiquidationSource.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 14: import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 15: import { TwabController, SPONSORSHIP_ADDRESS } from "pt-v5-twab-controller/TwabController.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { IERC4626 } from "openzeppelin/interfaces/IERC4626.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 5: import { SafeERC20, IERC20Permit } from "openzeppelin/token/ERC20/utils/SafeERC20.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 6: import { ERC20, IERC20, IERC20Metadata } from "openzeppelin/token/ERC20/ERC20.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 7: import { Math } from "openzeppelin/utils/math/Math.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 8: import { Ownable } from "owner-manager-contracts/Ownable.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 10: import { Claimable } from "./abstract/Claimable.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 11: import { TwabERC20 } from "./TwabERC20.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 13: import { ILiquidationSource } from "pt-v5-liquidator-interfaces/ILiquidationSource.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 14: import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 15: import { TwabController, SPONSORSHIP_ADDRESS } from "pt-v5-twab-controller/TwabController.sol"; /// @audit `More than one space around an assignment or other operator to align with another` 647: uint256 _liquidYield = _availableYieldBalance(totalAssets(), _totalDebt(_totalSupply))
4 | 5 | 6 | 7 | 8 | 10 | 11 | 13 | 14 | 15 | 4 | 5 | 6 | 7 | 8 | 10 | 11 | 13 | 14 | 15 | 647
File: pt-v5-vault/src/PrizeVaultFactory.sol /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { IERC20, IERC4626 } from "openzeppelin/token/ERC20/extensions/ERC4626.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 6: import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 8: import { PrizeVault } from "./PrizeVault.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { IERC20, IERC4626 } from "openzeppelin/token/ERC20/extensions/ERC4626.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 6: import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 8: import { PrizeVault } from "./PrizeVault.sol";
File: pt-v5-vault/src/TwabERC20.sol /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { ERC20 } from "openzeppelin/token/ERC20/ERC20.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 5: import { ERC20Permit } from "openzeppelin/token/ERC20/extensions/ERC20Permit.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 6: import { SafeCast } from "openzeppelin/utils/math/SafeCast.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 8: import { TwabController } from "pt-v5-twab-controller/TwabController.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { ERC20 } from "openzeppelin/token/ERC20/ERC20.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 5: import { ERC20Permit } from "openzeppelin/token/ERC20/extensions/ERC20Permit.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 6: import { SafeCast } from "openzeppelin/utils/math/SafeCast.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 8: import { TwabController } from "pt-v5-twab-controller/TwabController.sol";
File: pt-v5-vault/src/abstract/Claimable.sol /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { IClaimable } from "pt-v5-claimable-interface/interfaces/IClaimable.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 5: import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 7: import { HookManager } from "./HookManager.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 86: recipient = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }( /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 109: _hooks[_winner].implementation.afterClaimPrize{ gas: HOOK_GAS }( /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { IClaimable } from "pt-v5-claimable-interface/interfaces/IClaimable.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 5: import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 7: import { HookManager } from "./HookManager.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 86: recipient = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }( /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 109: _hooks[_winner].implementation.afterClaimPrize{ gas: HOOK_GAS }(
4 | 5 | 7 | 86 | 109 | 4 | 5 | 7 | 86 | 109
</details>File: pt-v5-vault/src/abstract/HookManager.sol /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { VaultHooks } from "../interfaces/IVaultHooks.sol"; /// @audit `Immediately inside parenthesis, brackets or braces, with the exception of single line function declarations.` 4: import { VaultHooks } from "../interfaces/IVaultHooks.sol";
The contract contains import statements for libraries or other contracts that are not utilized within the code. Excessive or unused imports can clutter the codebase, leading to inefficiency and potential confusion. Consider removing any imports that are not essential to the contract's functionality.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol /// @audit imported `TwabController` not used) 15: import { TwabController, SPONSORSHIP_ADDRESS } from "pt-v5-twab-controller/TwabController.sol";
if-statement can be converted to a ternaryThe ternary operator provides a shorthand way to write if / else statements.
It can improve readability and reduce the number of lines of code.
Traditional if / else statements, particularly those spanning only a one lines, could potentially be refactored using the ternary operator.
</details>File: pt-v5-vault/src/abstract/Claimable.sol 84: if (_hooks[_winner].useBeforeClaimPrize) { recipient = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }( _winner, _tier, _prizeIndex, _reward, _rewardRecipient ); } else { recipient = _winner; }
As of Solidity version 0.8.18, it is possible to use named mappings to clarify the purpose of each mapping in the code. It is recommended to use this feature for better code readability and maintainability.
More information: Solidity 0.8.18 Release Announcement
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/abstract/HookManager.sol 17: mapping(address => VaultHooks) internal _hooks;
override is unnecessaryIn Solidity version 0.8.8 and later, the use of the override keyword becomes superfluous when a function is overriding solely from an interface and the function isn't present in multiple base contracts.
Previously, the override keyword was required as an explicit indication to the compiler. However, this is no longer the case, and the extraneous use of the keyword can make the code less clean and more verbose.
Solidity documentation on Function Overriding.
<details> <summary><i>6 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 320: function decimals() public view override(ERC20, IERC20Metadata) returns (uint8) {
</details>File: pt-v5-vault/src/TwabERC20.sol 56: function balanceOf( address _account ) public view virtual override(ERC20) returns (uint256) { 63: function totalSupply() public view virtual override(ERC20) returns (uint256) { 76: function _mint(address _receiver, uint256 _amount) internal virtual override { 87: function _burn(address _owner, uint256 _amount) internal virtual override { 100: function _transfer(address _from, address _to, uint256 _amount) internal virtual override {
Setter functions should include a condition to check if the new value being assigned is different from the current value. This practice prevents redundant state changes and the emission of unnecessary events, ensuring that changes only occur when actual updates to the values are made.
This not only helps to maintain the integrity of the contract's state but also keeps the event logs cleaner and more meaningful by avoiding the recording of identical consecutive values. Such an approach can improve the clarity and efficiency of debugging and data tracking processes.
<details> <summary><i>7 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol /// @audit Missing `_claimer` check before state change 736: _setClaimer(_claimer); /// @audit Missing `_liquidationPair` check before state change 744: liquidationPair = _liquidationPair; /// @audit Missing `_yieldFeePercentage` check before state change 754: _setYieldFeePercentage(_yieldFeePercentage); /// @audit Missing `_yieldFeeRecipient` check before state change 760: _setYieldFeeRecipient(_yieldFeeRecipient); /// @audit Missing `_yieldFeePercentage` check before state change 951: yieldFeePercentage = _yieldFeePercentage; /// @audit Missing `_yieldFeeRecipient` check before state change 959: yieldFeeRecipient = _yieldFeeRecipient;
736 | 744 | 754 | 760 | 951 | 959
</details>File: pt-v5-vault/src/abstract/Claimable.sol /// @audit Missing `_claimer` check before state change 130: claimer = _claimer;
@title tagsNatSpec comments offer an intuitive way to understand the core purpose of a smart contract.
Especially, the @title tag serves as a brief descriptor of the contract's function or intent.
In this Solidity file, the @title directive seems to be overlooked.
Incorporating the @title tag gives readers a concise overview, thus enhancing clarity.
Refer to NatSpec Documentation
</details>File: pt-v5-vault/src/PrizeVault.sol 65: contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownable {
It's recommended to have full test coverage for all contracts. While 100% code coverage does not guarantee absence of bugs, it can catch simple bugs and reduce regressions during code modifications. Moreover, to achieve full coverage, authors often have to refactor their code into more modular components, each testable separately. This leads to lower interdependencies, and results in code that is easier to understand and audit.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: app/2024-03-pooltogether 1: All files
Large or complex code bases should include invariant fuzzing tests, such as those provided by Echidna. These tests require the identification of invariants that should not be violated under any circumstances, with the fuzzer testing various inputs and function calls to ensure these invariants always hold. This is especially important for code with a lot of inline-assembly, complicated math, or complex interactions between contracts. Despite having 100% code coverage, bugs can still occur due to the order of operations a user performs. Extensive and well-written invariant tests can significantly reduce this testing gap.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: app/2024-03-pooltogether 1: All files
Formal verification offers a mathematical proof confirming that your code operates as intended and is devoid of edge cases that may lead to unintended behavior. By leveraging this rigorous audit technique, you not only enhance the robustness of your code but also strengthen the trust of stakeholders in the safety of your contract.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: app/2024-03-pooltogether 1: All files
#0 - raymondfam
2024-03-13T06:56:49Z
Generic findings well elaborated: 26 L and 58 NC
#1 - c4-pre-sort
2024-03-13T06:56:56Z
raymondfam marked the issue as high quality report
#2 - c4-sponsor
2024-03-14T16:14:09Z
trmid (sponsor) confirmed
#3 - trmid
2024-03-15T20:35:42Z
[L-12] mitigation: https://github.com/GenerationSoftware/pt-v5-vault/pull/86
#4 - trmid
2024-03-15T20:53:34Z
[L-11] mitigation: https://github.com/GenerationSoftware/pt-v5-vault/pull/87
#5 - c4-judge
2024-03-18T02:58:24Z
hansfriese marked the issue as grade-a
#6 - c4-judge
2024-03-18T02:58:53Z
hansfriese marked the issue as selected for report
#7 - c4-judge
2024-03-18T04:58:16Z
hansfriese marked the issue as not selected for report
#8 - hansfriese
2024-03-18T05:00:59Z
Invalid [L-05] Code does not follow the best practice of check-effects-interaction
Known findings [L-01] Centralization risk for privileged functions The owner is a single point of failure and a centralization risk
[L-02] Subtraction in unchecked block is unsafe unchecked blocks with subtractions may underflow
[L-04] Some tokens may revert on large transfers ERC-20: Large transfers may revert
[L-06] Use abi.encodeCall() instead of abi.encodeWithSignature()/abi.encodeWithSelector() Use abi.encodeCall() instead of abi.encodeWithSignature()/abi.encodeWithSelector()
[L-07] Upgradable contracts not taken into account Consider making contracts Upgradeable
[L-10] Unchecked Return Values of the approve() Function Return values of approve() not checked
[L-12] Unsafe use of transfer()/transferFrom() with IERC20 [L-13] Unchecked Return Values of transfer()/transferFrom() ERC20: unsafe use of transfer()/transferFrom()
[L-15] Missing checks for address(0x0) when updating address state variables Missing checks for address(0x0) when updating address state variables
[L-16] Lack of Parameter Validation in Constructor/Initializer] Missing checks for address(0x0) in the constructor/initializer
...
Most findings are dupes of the known ones. I recommend filtering them when you submit with your bot results.
#9 - c4-judge
2024-03-18T05:01:30Z
hansfriese marked the issue as grade-b
#10 - c4-judge
2024-03-18T12:52:32Z
hansfriese marked the issue as grade-a
#11 - trmid
2024-03-18T19:10:16Z
[N-37] mitigation: https://github.com/GenerationSoftware/pt-v5-vault/pull/89
#12 - trmid
2024-03-18T19:36:06Z
[N-22] mitigation: https://github.com/GenerationSoftware/pt-v5-vault/pull/90
#13 - trmid
2024-03-18T20:18:52Z
[N-20] mitigation: https://github.com/GenerationSoftware/pt-v5-vault/pull/91
#14 - trmid
2024-03-18T20:22:56Z
[N-15] mitigation: https://github.com/GenerationSoftware/pt-v5-vault/pull/92
🌟 Selected for report: slvDev
Also found by: 0x11singh99, 0xhacksmithh, SAQ, SY_S, albahaca, dharma09, hunter_w3b, shamsulhaq123, unique
191.6119 USDC - $191.61
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G-01] | Use assembly for Efficient Event Emission | 14 | 5320 |
| [G-02] | Enable IR-based code generation | 6 | - |
| [G-03] | Use immutables variables directly, instead of cache them in stack | 2 | 0 |
| [G-04] | Assembly: Check msg.sender using xor and the scratch space | 4 | 84 |
| [G-05] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct | 2 | - |
| [G-06] | Assembly: Use scratch space when building emitted events with two data arguments | 3 | 45 |
| [G-07] | Stack variable is only used once | 4 | 12 |
| [G-08] | abi.encodePacked is more gas efficient than abi.encode | 1 | 200 |
| [G-09] | Use assembly to write mutable storage values | 6 | 66 |
| [G-10] | Use revert() to gain maximum gas savings | 24 | 1200 |
| [G-11] | >= costs less gas than > | 1 | 6 |
| [G-12] | Consider pre-calculating the address of address(this) to save gas | 25 | 0 |
| [G-13] | Call msg.sender directly instead of caching it | 1 | 14 |
| [G-14] | Use unchecked for Non-Loop Increment/Decrement Operations | 1 | 30 |
| [G-15] | Constructors can be marked payable | 3 | 72 |
| [G-16] | Reduce gas usage by moving to Solidity 0.8.19 or later | 1 | - |
| [G-17] | Reduce deployment costs by tweaking contracts' metadata | 6 | 63600 |
| [G-18] | Use Pre-Increment/Decrement (++i/--i) to Save Gas | 1 | 5 |
| [G-19] | Nesting if-statements is cheaper than using && | 1 | 6 |
| [G-20] | Avoid transferring amounts of zero in order to save gas | 1 | 200 |
| [G-21] | >= costs less gas than > | 6 | 18 |
| [G-22] | Use unchecked for Math Operations if they already checked | 5 | 425 |
| [G-23] | Using private rather than public, saves gas | 16 | 352000 |
| [G-24] | Use Assembly for Hash Calculations | 1 | 1005 |
| [G-25] | Refactor duplicated require()/revert() checks to save gas | 2 | - |
| [G-26] | Use Assembly for Efficient Memory Management in Multiple External Calls | 10 | 26000 |
| [G-27] | Cached Global Variables | 1 | 12 |
| [G-28] | Simple checks for zero uint can be done using assembly to save gas | 8 | 32 |
| [G-29] | Consider using solady's FixedPointMathLib | 1 | - |
| [G-30] | Avoid Zero to Non-Zero Storage Writes Where Possible | 2 | 44200 |
| [G-31] | Use unchecked for division which do not divide by -X sonce they can't overflow | 1 | 20 |
| [G-32] | Use solady library where possible to save gas | 6 | 8000 |
| [G-33] | Use uint256(1)/uint256(2) instead of true/false to save gas for changes | 1 | 17000 |
| [G-34] | Mark Functions That Revert For Normal Users As payable | 8 | 168 |
| [G-35] | Prefer private over public for Constants to Save Gas | 4 | 12000 |
| [G-36] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 2 | 226 |
| [G-37] | Using bools for storage incurs overhead | 1 | 100 |
| [G-38] | Optimize Gas by Using Only Named Returns | 40 | 1760 |
| [G-39] | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 19 | 114 |
| [G-40] | Use assembly to check for address(0) | 7 | 406 |
| [G-41] | internal/private functions only called once can be inlined to save gas | 2 | 40 |
| [G-42] | Declare immutable as private to save gas | 4 | 12000 |
| [G-43] | Optimize names to save gas | 5 | 100 |
| [G-44] | Avoid updating storage when the value hasn't changed | 7 | 5600 |
| [G-45] | Optimize External Calls with Assembly for Memory Efficiency | 24 | 5280 |
Total: 290 instances of 45 issues with 557366 gas saved
assembly for Efficient Event EmissionTo efficiently emit events, consider utilizing assembly by making use of scratch space and the free memory pointer. This approach can potentially avoid the costs associated with memory expansion.
However, it's crucial to cache and restore the free memory pointer for safe optimization. Good examples of such practices can be found in well-optimized Solady's codebases. Please review your code and consider the potential gas savings of this approach.
<details> <summary><i>14 issue instances in 5 files:</i></summary>File: pt-v5-vault/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)
562 | 621 | 697 | 747 | 876 | 909 | 952 | 960
File: pt-v5-vault/src/PrizeVaultFactory.sol 123: emit NewPrizeVault( _vault, _yieldVault, _prizePool, _name, _symbol )
File: pt-v5-vault/src/TwabERC20.sol 78: emit Transfer(address(0), _receiver, _amount) 89: emit Transfer(_owner, address(0), _amount) 102: emit Transfer(_from, _to, _amount)
File: pt-v5-vault/src/abstract/Claimable.sol 131: emit ClaimerSet(_claimer)
</details>File: pt-v5-vault/src/abstract/HookManager.sol 31: emit SetHooks(msg.sender, hooks)
The --via-ir command line option activates the IR-based code generator in Solidity, which is designed to enable powerful optimization passes that can span across functions. The end result may be a contract that requires less gas to execute its functions.
We recommend you enable this feature, run tests, and benchmark the gas usage of your contract to evaluate if it leads to any tangible gas savings. Experimenting with this feature could lead to a more gas-efficient contract.
<details> <summary><i>6 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol File: pt-v5-vault/src/PrizeVaultFactory.sol File: pt-v5-vault/src/TwabERC20.sol File: pt-v5-vault/src/abstract/Claimable.sol File: pt-v5-vault/src/abstract/HookManager.sol File: pt-v5-vault/src/interfaces/IVaultHooks.sol
immutables variables directly, instead of cache them in stackCaching immutables variable in stack is not necessary, and it will cost more gas.
</details>File: pt-v5-vault/src/PrizeVault.sol 599: uint256 _yieldBuffer = yieldBuffer 825: uint256 _yieldBuffer = yieldBuffer
msg.sender using xor and the scratch spaceSee this prior finding for details on the conversion
<details> <summary><i>4 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 261: if (msg.sender != liquidationPair) { 269: if (msg.sender != yieldFeeRecipient) { 532: if (_owner != msg.sender) {
</details>File: pt-v5-vault/src/abstract/Claimable.sol 53: if (msg.sender != claimer) revert CallerNotClaimer(msg.sender, claimer);
address/ID mappings can be combined into a single mapping of an address/ID to a structCombining multiple address/ID mappings into a single mapping to a struct can lead to gas savings. By refactoring multiple mappings into a singular mapping with a struct, you can save on storage slots, which in turn can reduce the gas cost in certain operations. Prioritize this refactor if optimizing gas is a primary concern for your contract's operations.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVaultFactory.sol 69: mapping(address vault => bool deployedByFactory) public deployedVaults 72: mapping(address deployer => uint256 nonce) public deployerNonces
Using the scratch space for more than one, but at most two words worth of data (non-indexed arguments) will save gas over needing Solidity's abi memory expansion used for emitting normally.
<details> <summary><i>3 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 562: emit Sponsor(_owner, _assets, _shares) 697: emit TransferYieldOut(msg.sender, _tokenOut, _receiver, _amountOut, _yieldFee)
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol 123: emit NewPrizeVault( _vault, _yieldVault, _prizePool, _name, _symbol )
If the variable is only accessed once, it's cheaper to use the assigned value directly that one time, and save the 3 gas the extra stack assignment would spend
<details> <summary><i>4 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol /// @audit - `totalDebt_` variable 376: uint256 totalDebt_ = _totalDebt(_totalSupply) /// @audit - `_yieldVaultShares` variable 865: uint256 _yieldVaultShares = yieldVault.previewDeposit(_assetsWithDust) /// @audit - `_assetsUsed` variable 866: uint256 _assetsUsed = yieldVault.mint(_yieldVaultShares, address(this)) /// @audit - `_yieldVaultShares` variable 934: uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets)
abi.encodePacked is more gas efficient than abi.encodeabi.encode() pads all elementary types to 32 bytes, whereas abi.encodePacked() will only use the minimal required memory to encode the data.
<details> <summary><i>1 issue instances in 1 files:</i></summary>function testPacking() public pure returns (bytes memory) { // return abi.encode("string"); // 1120 gas // return abi.encodePacked("string"); // 913 gas }
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol 103: abi.encode(msg.sender, deployerNonces[msg.sender]++)
assembly to write mutable storage valuesWriting to storage using assembly is more gas efficient.
<details> <summary><i>6 issue instances in 4 files:</i></summary>function writeStorage() external { // storageNumber = 10; // 2358 gas // assembly { // sstore(storageNumber.slot, 10) // 2350 gas // } // storageAddr = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc3; // 2411 gas // assembly { // sstore(storageAddr.slot, 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc3) // 2350 gas // } }
File: pt-v5-vault/src/PrizeVault.sol 745: liquidationPair = _liquidationPair 951: yieldFeePercentage = _yieldFeePercentage 959: yieldFeeRecipient = _yieldFeeRecipient
File: pt-v5-vault/src/PrizeVaultFactory.sol 121: deployedVaults[address(_vault)] = true
File: pt-v5-vault/src/abstract/Claimable.sol 130: claimer = _claimer
</details>File: pt-v5-vault/src/abstract/HookManager.sol 30: _hooks[msg.sender] = hooks
revert() to gain maximum gas savingsIf you dont need Error messages, or you want gain maximum gas savings - revert() is a cheapest way to revert transaction in terms of gas.
<details> <summary><i>24 issue instances in 3 files:</i></summary>revert(); // 117 gas require(false); // 132 gas revert CustomError(); // 157 gas assert(false); // 164 gas revert("Custom Error"); // 406 gas require(false, "Custom Error"); // 421 gas
File: pt-v5-vault/src/PrizeVault.sol 262: revert CallerNotLP(msg.sender, liquidationPair) 270: revert CallerNotYieldFeeRecipient(msg.sender, yieldFeeRecipient) 300: revert YieldVaultZeroAddress() 301: revert OwnerZeroAddress() 458: revert ZeroTotalAssets() 533: revert PermitCallerNotOwner(msg.sender, _owner) 612: revert MintZeroShares() 615: revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance) 665: revert LiquidationAmountOutZero() 680: revert LiquidationExceedsAvailable(_amountOut + _yieldFee, _availableYield) 694: revert LiquidationTokenOutNotSupported(_tokenOut) 710: revert LiquidationTokenInNotPrizeToken(_tokenIn, _prizeToken) 743: revert LPZeroAddress() 844: revert MintZeroShares() 845: revert DepositZeroAssets() 874: revert LossyDeposit(totalAssets(), totalDebt()) 894: revert WithdrawZeroAssets() 895: revert BurnZeroShares() 949: revert YieldFeePercentageExceedsMax(_yieldFeePercentage, MAX_YIELD_FEE)
262 | 270 | 300 | 301 | 458 | 533 | 612 | 615 | 665 | 680 | 694 | 710 | 743 | 844 | 845 | 874 | 894 | 895 | 949
File: pt-v5-vault/src/TwabERC20.sol 47: revert TwabControllerZeroAddress()
</details>File: pt-v5-vault/src/abstract/Claimable.sol 53: revert CallerNotClaimer(msg.sender, claimer) 65: revert PrizePoolZeroAddress() 97: revert ClaimRecipientZeroAddress() 129: revert ClaimerZeroAddress()
>= costs less gas than >The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas. If < is being used, the condition can be inverted. In cases where a for-loop is being used, one can count down rather than up, in order to use the optimal operator.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 684: if (_yieldFee > 0) {
address(this) to save gasUse foundry's script.sol or solady's LibRlp.sol to save the value in a constant, which will avoid having to spend gas to push the value on the stack every time it's used.
<details> <summary><i>25 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 337: return yieldVault.convertToAssets(yieldVault.balanceOf(address(this))) + _asset.balanceOf(address(this)); 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)); 539: if (_asset.allowance(_owner, address(this)) != _assets) { 540: IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s); 558: if (twabController.delegateOf(address(this), _owner) != SPONSORSHIP_ADDRESS) { 634: if (_tokenOut == address(this)) { 639: _maxAmountOut = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this)); 691: } else if (_tokenOut == address(this)) { 713: prizePool.contributePrizeTokens(address(this), _amountIn); 726: return (_tokenOut == address(_asset) || _tokenOut == address(this)) && _liquidationPair == liquidationPair; 747: emit LiquidationPairSet(address(this), address(_liquidationPair)); 856: address(this), 861: uint256 _assetsWithDust = _asset.balanceOf(address(this)); 866: uint256 _assetsUsed = yieldVault.mint(_yieldVaultShares, address(this)); 922: return yieldVault.convertToAssets(yieldVault.maxRedeem(address(this))); 931: uint256 _latentAssets = _asset.balanceOf(address(this)); 936: yieldVault.redeem(_yieldVaultShares, address(this), address(this)); 936: yieldVault.redeem(_yieldVaultShares, address(this), address(this)); 938: if (_receiver != address(this)) {
337 | 337 | 382 | 383 | 405 | 416 | 539 | 540 | 558 | 634 | 639 | 691 | 713 | 726 | 747 | 856 | 861 | 866 | 922 | 931 | 936 | 936 | 938
</details>File: pt-v5-vault/src/TwabERC20.sol 59: return twabController.balanceOf(address(this), _account); 64: return twabController.totalSupply(address(this));
msg.sender directly instead of caching itIn the instance below, instead of caching msg.sender and incurring unnecessary stack manipulation, we can call msg.sender directly.
<details> <summary><i>1 issue instances in 1 files:</i></summary>/// 3047 gas function test() external { internalFunc(msg.sender); internalFunc(msg.sender); internalFunc(msg.sender); } // 3061 gas function test() external { address callerLocal = msg.sender; internalFunc(callerLocal); internalFunc(callerLocal); internalFunc(callerLocal); }
</details>File: pt-v5-vault/src/PrizeVault.sol 553: address _owner = msg.sender
unchecked for Non-Loop Increment/Decrement OperationsDisclaimer: You should be sure that underflow is not possible
Using unchecked increments can save gas by bypassing the built-in overflow checks.
This can save 30-40 gas per iteration.
It is recommended to use unchecked increments when overflow is not possible.
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol 103: deployerNonces[msg.sender]++
payablePayable functions cost less gas to execute, since the compiler does not have to add extra checks to ensure that a payment wasn't provided.
A constructor can safely be marked as payable, since only the deployer would be able to pass funds, and the project itself would not pass any funds.
T
his could save an average of about 21 gas per call, in addition to the extra deployment cost.
File: pt-v5-vault/src/PrizeVault.sol 289: constructor( string memory name_, string memory symbol_, IERC4626 yieldVault_, PrizePool prizePool_, address claimer_, address yieldFeeRecipient_, uint32 yieldFeePercentage_, uint256 yieldBuffer_, address owner_ ) TwabERC20(name_, symbol_, prizePool_.twabController()) Claimable(prizePool_, claimer_) Ownable(owner_) {
File: pt-v5-vault/src/TwabERC20.sol 42: constructor( string memory name_, string memory symbol_, TwabController twabController_ ) ERC20(name_, symbol_) ERC20Permit(name_) {
</details>File: pt-v5-vault/src/abstract/Claimable.sol 64: constructor(PrizePool prizePool_, address claimer_) {
See this link for the full details. Additionally, every new release has new optimizations, which will save gas.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/abstract/HookManager.sol 2: pragma solidity ^0.8.0;
The Solidity compiler appends 53 bytes of metadata to the smart contract code which translates to an extra 10,600 gas (200 per bytecode) + the calldata cost (16 gas per non-zero bytes, 4 gas per zero-byte). This translates to up to 848 additional gas in calldata cost. One way to reduce this cost is by optimizing the IPFS hash that gets appended to the smart contract code.
Why is this important?
Options to Reduce Gas:
--no-cbor-metadata compiler option to exclude metadata, but this might affect contract verification.File: pt-v5-vault/src/PrizeVault.sol 1: Consider optimizing the IPFS hash during deployment.
File: pt-v5-vault/src/PrizeVaultFactory.sol 1: Consider optimizing the IPFS hash during deployment.
File: pt-v5-vault/src/TwabERC20.sol 1: Consider optimizing the IPFS hash during deployment.
File: pt-v5-vault/src/abstract/Claimable.sol 1: Consider optimizing the IPFS hash during deployment.
File: pt-v5-vault/src/abstract/HookManager.sol 1: Consider optimizing the IPFS hash during deployment.
</details>File: pt-v5-vault/src/interfaces/IVaultHooks.sol 1: Consider optimizing the IPFS hash during deployment.
Using pre-increment (++i) or pre-decrement (--i) operators is more gas-efficient compared to their post counterparts (i++ or i--). This is because pre-increment/decrement operators avoid the need for an additional temporary variable that stores the original value of the iterator. This subtle difference results in saving of around 5 gas units per operation, which can accumulate to substantial savings in gas costs in contracts with frequent increment/decrement operations.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVaultFactory.sol 103: salt: keccak256(abi.encode(msg.sender, deployerNonces[msg.sender]++))
if-statements is cheaper than using &&Optimization of condition checks in your smart contract is a crucial aspect in ensuring gas efficiency. Specifically, substituting multiple && checks with nested if statements can lead to substantial gas savings.
When evaluating multiple conditions within a single if statement using the && operator, each condition will consume gas even if a preceding condition fails. However, if these checks are broken down into nested if statements, execution halts as soon as a condition fails, saving the gas that would have been consumed by subsequent checks.
This practice is especially beneficial in scenarios where the if statement isn't followed by an else statement. The reason being, when an else statement is present, all conditions must be checked regardless to determine the correct branch of execution.
By reworking your code to utilize nested if statements, you can optimize gas usage, reduce execution cost, and enhance your contract's performance.
</details>File: pt-v5-vault/src/PrizeVault.sol 776: if (success && encodedDecimals.length >= 32) {
Performing token or Ether transfers with a zero amount may result in unnecessary gas consumption. The absence of a zero-amount check before a transfer or send operation can lead to wasted gas, as the state of the contract remains the same even if the amount is zero. Adding a conditional check for zero amounts can prevent these costly, unnecessary operations, thereby optimizing the contract's gas usage.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/TwabERC20.sol /// @audit `SafeCast.toUint96(_amount)` has not been checked for zero value before transfer 101: twabController.transfer(_from, _to, SafeCast.toUint96(_amount));
>= costs less gas than >The Solidity compiler requires fewer opcodes when the >= operator is used in place of the > operator.
Specifically, the compiler uses GT and ISZERO opcodes for > but only requires LT for >=, saving 3 gas.
Thus, wherever applicable, it's recommended to use >= instead of > to enhance gas efficiency in your code. Same applies for <= and <.
File: pt-v5-vault/src/PrizeVault.sol 422: if (_ownerShares > _maxWithdraw) { 615: if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance); 679: if (_amountOut + _yieldFee > _availableYield) { 874: if (totalAssets() < totalDebt()) revert LossyDeposit(totalAssets(), totalDebt()); 932: if (_assets > _latentAssets) { 948: if (_yieldFeePercentage > MAX_YIELD_FEE) {
422 | 615 | 679 | 874 | 932 | 948
</details>unchecked for Math Operations if they already checkedSome subtraction operations in the contract have implicit checks that prevent underflow.
To optimize gas, consider wrapping such operations in an unchecked block.
Always review the logic thoroughly before making changes to ensure the safety of operations.
</details>File: pt-v5-vault/src/PrizeVault.sol /// @audit - mathematical operation `_maxYieldVaultDeposit - _latentBalance` checked above in line: /// 384: if (_latentBalance >= _maxYieldVaultDeposit) { 388: _maxDeposit = _maxYieldVaultDeposit - _latentBalance; /// @audit - mathematical operation `_amountOut + _yieldFee` checked above in line: /// 679: if (_amountOut + _yieldFee > _availableYield) { 680: revert LiquidationExceedsAvailable(_amountOut + _yieldFee, _availableYield); /// @audit - mathematical operation `_totalAssets - totalDebt_` checked above in line: /// 809: if (totalDebt_ >= _totalAssets) { 813: return _totalAssets - totalDebt_; /// @audit - mathematical operation `totalYieldBalance_ - _yieldBuffer` checked above in line: /// 826: if (totalYieldBalance_ >= _yieldBuffer) { 828: return totalYieldBalance_ - _yieldBuffer; /// @audit - mathematical operation `_assets - _latentAssets` checked above in line: /// 932: if (_assets > _latentAssets) { 934: uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets);
private rather than public, saves gasPublic storage variables increase the contract's size due to the implicit generation of public getter functions. This makes the contract larger and could increase deployment and interaction costs.
If you do not require other contracts to read these variables, consider making them private.
Example:
<details> <summary><i>16 issue instances in 4 files:</i></summary>/// 145426 gas to deploy contract PublicState { address public first; address public second; } /// 77126 gas to deploy contract PrivateState { address private first; address private second; }
File: pt-v5-vault/src/PrizeVault.sol 74: uint32 public constant FEE_PRECISION = 1e9; 80: uint32 public constant MAX_YIELD_FEE = 9e8; 112: uint256 public immutable yieldBuffer; 115: IERC4626 public immutable yieldVault; 119: uint32 public yieldFeePercentage; 122: address public yieldFeeRecipient; 125: uint256 public yieldFeeBalance; 128: address public liquidationPair;
74 | 80 | 112 | 115 | 119 | 122 | 125 | 128
File: pt-v5-vault/src/PrizeVaultFactory.sol 63: uint256 public constant YIELD_BUFFER = 1e5; 66: PrizeVault[] public allVaults; 69: mapping(address vault => bool deployedByFactory) public deployedVaults; 72: mapping(address deployer => uint256 nonce) public deployerNonces;
File: pt-v5-vault/src/TwabERC20.sol 26: TwabController public immutable twabController;
</details>File: pt-v5-vault/src/abstract/Claimable.sol 21: uint24 public constant HOOK_GAS = 150_000; 24: PrizePool public immutable prizePool; 27: address public claimer;
In certain cases, using inline assembly to calculate hashes can lead to significant gas savings. Solidity's built-in keccak256 function is convenient but costs more gas than the equivalent assembly code. However, it's important to note that using assembly should be done with care as it's less readable and could increase the risk of introducing errors.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVaultFactory.sol 103: salt: keccak256(abi.encode(msg.sender, deployerNonces[msg.sender]++))
Duplicate require()/revert() checks can be refactored into a modifier or function, saving deployment costs.
<details> <summary><i>2 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 612: if (_shares == 0) revert MintZeroShares(); 844: if (_shares == 0) revert MintZeroShares();
When making multiple external calls in a Solidity contract, it may be more efficient to use inline assembly to reuse the memory space allocated for function arguments and return data. This can prevent unnecessary memory expansion and reduce gas costs.
Example:
<details> <summary><i>10 issue instances in 1 files:</i></summary>contract Solidity { // cost: 7262 function call(address calledAddress) external pure returns(uint256) { Called called = Called(calledAddress); uint256 res1 = called.add(1, 2); uint256 res2 = called.add(3, 4); uint256 res = res1 + res2; return res; } } contract Assembly { // cost: 5281 function call(address calledAddress) external view returns(uint256) { assembly { // check that calledAddress has code deployed to it if iszero(extcodesize(calledAddress)) { revert(0x00, 0x00) } // first call mstore(0x00, hex"771602f7") mstore(0x04, 0x01) mstore(0x24, 0x02) let success := staticcall(gas(), calledAddress, 0x00, 0x44, 0x60, 0x20) if iszero(success) { revert(0x00, 0x00) } let res1 := mload(0x60) // second call mstore(0x04, 0x03) mstore(0x24, 0x4) success := staticcall(gas(), calledAddress, 0x00, 0x44, 0x60, 0x20) if iszero(success) { revert(0x00, 0x00) } let res2 := mload(0x60) // add results let res := add(res1, res2) // return data mstore(0x60, res) return(0x60, 0x20) } } }
File: pt-v5-vault/src/PrizeVault.sol /// @audit function `maxDeposit()` has multiple external calls 382: uint256 _latentBalance = _asset.balanceOf(address(this)); 383: uint256 _maxYieldVaultDeposit = yieldVault.maxDeposit(address(this)); /// @audit function `_depositAndMint()` has multiple external calls 854: _asset.safeTransferFrom( _caller, address(this), _assets ); 861: uint256 _assetsWithDust = _asset.balanceOf(address(this)); 862: _asset.approve(address(yieldVault), _assetsWithDust); 865: uint256 _yieldVaultShares = yieldVault.previewDeposit(_assetsWithDust); 869: _asset.approve(address(yieldVault), 0); /// @audit function `_withdraw()` has multiple external calls 931: uint256 _latentAssets = _asset.balanceOf(address(this)); 934: uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets); 936: yieldVault.redeem(_yieldVaultShares, address(this), address(this));
382 | 383 | 854 | 861 | 862 | 865 | 869 | 931 | 934 | 936
</details>Storing global variables in local storage might appear as a useful caching mechanism. However, in Solidity, accessing global variables directly is often more gas-efficient than caching them. Consider removing these redundant assignments and use the global variables directly.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 553: address _owner = msg.sender;
uint can be done using assembly to save gasThe usage of inline assembly to check if variable is the zero can save gas compared to traditional require or if statement checks.
// gas 396 gas | 399 gas assembly { if iszero(value) { // use iszero(iszero(value)) for != add 3 gas revert(0, 0) } } require(value != 0); // 401 gas require(value == 0); // 401 gas <details> <summary><i>8 issue instances in 1 files:</i></summary> ```solidity File: pt-v5-vault/src/PrizeVault.sol 458: if (_totalAssets == 0) revert ZeroTotalAssets(); 612: if (_shares == 0) revert MintZeroShares(); 665: if (_amountOut == 0) revert LiquidationAmountOutZero(); 672: if (_yieldFeePercentage != 0) { // The yield fee is calculated as a portion of the total yield being consumed, such that // `total = amountOut + yieldFee` and `yieldFee / total = yieldFeePercentage`. _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut; 844: if (_shares == 0) revert MintZeroShares(); 845: if (_assets == 0) revert DepositZeroAssets(); 894: if (_assets == 0) revert WithdrawZeroAssets(); 895: if (_shares == 0) revert BurnZeroShares();
458 | 612 | 665 | 672 | 844 | 845 | 894 | 895
</details>FixedPointMathLibUtilizing gas-optimized math functions from libraries like Solady can lead to more efficient smart contracts. This is particularly beneficial in contracts where these operations are frequently used.
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVault.sol 675: _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut;
Changing a storage variable from zero to non-zero costs 22,100 gas in total. (20,000 gas for a zero to non-zero write and 2,100 for a cold storage access) Consider using non-zero architecture to avoid high gas costs for zero to non-zero storage writes.
Example:
<details> <summary><i>2 issue instances in 1 files:</i></summary>- uint256 public counter = 0; // rewrite this costs more + uint256 public counter = 1; // rewrite this costs less
</details>File: pt-v5-vault/src/PrizeVault.sol 617: yieldFeeBalance -= _yieldFeeBalance; 951: yieldFeePercentage = _yieldFeePercentage;
unchecked for division which do not divide by -X sonce they can't overflowSolidity introduced the unchecked block in version 0.8.0 as a measure to provide control over arithmetic operations.
Any operation inside this block will not trigger the built-in overflow and underflow checks, thus saving gas costs.
Since a division operation between two uints (unsigned integers) can never result in an overflow or underflow, it's an ideal candidate for the use of unchecked {} block.
This practice enables optimal gas usage without risking any arithmetic anomalies.
</details>File: pt-v5-vault/src/PrizeVault.sol 675: _yieldFee = (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut;
The following OpenZeppelin imports have a Solady equivalent, as such they can be used to save GAS as Solady modules have been specifically designed to be as GAS efficient as possible.
<details> <summary><i>6 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 3: import { IERC4626 } from "openzeppelin/interfaces/IERC4626.sol"; 5: import { SafeERC20, IERC20Permit } from "openzeppelin/token/ERC20/utils/SafeERC20.sol"; 6: import { ERC20, IERC20, IERC20Metadata } from "openzeppelin/token/ERC20/ERC20.sol";
File: pt-v5-vault/src/PrizeVaultFactory.sol 3: import { IERC20, IERC4626 } from "openzeppelin/token/ERC20/extensions/ERC4626.sol";
</details>File: pt-v5-vault/src/TwabERC20.sol 3: import { ERC20 } from "openzeppelin/token/ERC20/ERC20.sol"; 5: import { ERC20Permit } from "openzeppelin/token/ERC20/extensions/ERC20Permit.sol";
uint256(1)/uint256(2) instead of true/false to save gas for changesBoolean variables in Solidity are more expensive than uint256 or any type that takes up a full word, due to additional gas costs associated with write operations.
When using boolean variables, each write operation emits an extra SLOAD to read the slot's contents, replace the bits taken up by the boolean, and then write back.
This process cannot be disabled and leads to extra gas consumption.
By using uint256(1) and uint256(2) for representing true and false states, you can avoid a Gwarmaccess (100 gas) cost and also avoid a Gsset (20000 gas) cost when changing from false to true, after having been true in the past.
This approach helps in optimizing gas usage, making your contract more cost-effective.
Usage in OpenZeppelin ReentrancyGuard.sol
<details> <summary><i>1 issue instances in 1 files:</i></summary></details>File: pt-v5-vault/src/PrizeVaultFactory.sol 69: mapping(address vault => bool deployedByFactory) public deployedVaults;
payableFunctions guaranteed to revert when called by normal users can be marked payable.
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function.
Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.
The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.
<details> <summary><i>8 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 611: function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient { 659: function transferTokensOut( address, address _receiver, address _tokenOut, uint256 _amountOut ) public virtual onlyLiquidationPair returns (bytes memory) { 703: function verifyTokensIn( address _tokenIn, uint256 _amountIn, bytes calldata ) external onlyLiquidationPair { 735: function setClaimer(address _claimer) external onlyOwner { 742: function setLiquidationPair(address _liquidationPair) external onlyOwner { 753: function setYieldFeePercentage(uint32 _yieldFeePercentage) external onlyOwner { 759: function setYieldFeeRecipient(address _yieldFeeRecipient) external onlyOwner {
611 | 659 | 703 | 735 | 742 | 753 | 759
</details>File: pt-v5-vault/src/abstract/Claimable.sol 76: function claimPrize( address _winner, uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256) {
private over public for Constants to Save GasUsing private instead of public for constants can save gas.
The compiler doesn't need to create non-payable getter functions for deployment calldata, store the bytes of the value outside of where it's used, or add another entry to the method ID table, saving 3406-3606 gas in deployment.
If needed, the values can be read from the verified contract source code, or a single getter function that returns a tuple of the values of all currently public constants can be implemented.
<details> <summary><i>4 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 74: uint32 public constant FEE_PRECISION = 1e9; 80: uint32 public constant MAX_YIELD_FEE = 9e8;
File: pt-v5-vault/src/PrizeVaultFactory.sol 63: uint256 public constant YIELD_BUFFER = 1e5;
</details>File: pt-v5-vault/src/abstract/Claimable.sol 21: uint24 public constant HOOK_GAS = 150_000;
<x> += <y> costs more gas than <x> = <x> + <y> for state variablesUsing the addition operator instead of plus-equals saves gas.
Replace <x> += <y> or <x> -= <y> with <x> = <x> + <y> or <x> = <x> - <y>.
</details>File: pt-v5-vault/src/PrizeVault.sol 617: yieldFeeBalance -= _yieldFeeBalance; 685: yieldFeeBalance += _yieldFee;
bools for storage incurs overheadUtilizing booleans for storage is less gas-efficient compared to using types that consume a full word like uint256. Every write operation on a boolean necessitates an extra SLOAD operation to read the slot's current value, modify the boolean bits, and then write back. This additional step is the compiler's measure against contract upgrades and pointer aliasing.
To enhance gas efficiency, consider using uint256(0) for false and uint256(1) for true, bypassing the extra Gwarmaccess (100 gas) incurred by the SLOAD.
</details>File: pt-v5-vault/src/PrizeVaultFactory.sol 69: mapping(address vault => bool deployedByFactory) public deployedVaults;
The Solidity compiler can generate more efficient bytecode when using named returns. It's recommended to replace anonymous returns with named returns for potential gas savings.
Example:
<details> <summary><i>40 issue instances in 6 files:</i></summary>/// 985 gas cost function add(uint256 x, uint256 y) public pure returns (uint256) { return x + y; } /// 941 gas cost function addNamed(uint256 x, uint256 y) public pure returns (uint256 res) { res = x + y; }
File: pt-v5-vault/src/PrizeVault.sol 320: function decimals() public view override(ERC20, IERC20Metadata) returns (uint8) { 329: function asset() external view returns (address) { 336: function totalAssets() public view returns (uint256) { 341: function convertToShares(uint256 _assets) public view returns (uint256) { 355: function convertToAssets(uint256 _shares) public view returns (uint256) { 374: function maxDeposit(address) public view returns (uint256) { 397: function maxMint(address _owner) public view returns (uint256) { 404: function maxWithdraw(address _owner) public view returns (uint256) { 415: function maxRedeem(address _owner) public view returns (uint256) { 441: function previewDeposit(uint256 _assets) public pure returns (uint256) { 447: function previewMint(uint256 _shares) public pure returns (uint256) { 454: function previewWithdraw(uint256 _assets) public view returns (uint256) { 470: function previewRedeem(uint256 _shares) public view returns (uint256) { 475: function deposit(uint256 _assets, address _receiver) external returns (uint256) { 482: function mint(uint256 _shares, address _receiver) external returns (uint256) { 489: function withdraw( uint256 _assets, address _receiver, address _owner ) external returns (uint256) { 500: function redeem( uint256 _shares, address _receiver, address _owner ) external returns (uint256) { 524: function depositWithPermit( uint256 _assets, address _owner, uint256 _deadline, uint8 _v, bytes32 _r, bytes32 _s ) external returns (uint256) { 552: function sponsor(uint256 _assets) external returns (uint256) { 573: function totalDebt() public view returns (uint256) { 584: function totalYieldBalance() public view returns (uint256) { 591: function availableYieldBalance() public view returns (uint256) { 597: function currentYieldBuffer() external view returns (uint256) { 631: function liquidatableBalanceOf(address _tokenOut) public view returns (uint256) { 659: function transferTokensOut( address, address _receiver, address _tokenOut, uint256 _amountOut ) public virtual onlyLiquidationPair returns (bytes memory) { 717: function targetOf(address) external view returns (address) { 722: function isLiquidationPair( address _tokenOut, address _liquidationPair ) external view returns (bool) { 772: function _tryGetAssetDecimals(IERC20 asset_) internal view returns (bool, uint8) { 790: function _totalDebt(uint256 _totalSupply) internal view returns (uint256) { 798: function _twabSupplyLimit(uint256 _totalSupply) internal pure returns (uint256) { 808: function _totalYieldBalance(uint256 _totalAssets, uint256 totalDebt_) internal pure returns (uint256) { 823: function _availableYieldBalance(uint256 _totalAssets, uint256 totalDebt_) internal view returns (uint256) { 921: function _maxYieldVaultWithdraw() internal view returns (uint256) {
320 | 329 | 336 | 341 | 355 | 374 | 397 | 404 | 415 | 441 | 447 | 454 | 470 | 475 | 482 | 489 | 500 | 524 | 552 | 573 | 584 | 591 | 597 | 631 | 659 | 717 | 722 | 772 | 790 | 798 | 808 | 823 | 921
File: pt-v5-vault/src/PrizeVaultFactory.sol 92: function deployVault( string memory _name, string memory _symbol, IERC4626 _yieldVault, PrizePool _prizePool, address _claimer, address _yieldFeeRecipient, uint32 _yieldFeePercentage, address _owner ) external returns (PrizeVault) { 136: function totalVaults() external view returns (uint256) {
File: pt-v5-vault/src/TwabERC20.sol 56: function balanceOf( address _account ) public view virtual override(ERC20) returns (uint256) { 63: function totalSupply() public view virtual override(ERC20) returns (uint256) {
File: pt-v5-vault/src/abstract/Claimable.sol 76: function claimPrize( address _winner, uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256) {
File: pt-v5-vault/src/abstract/HookManager.sol 22: function getHooks(address account) external view returns (VaultHooks memory) {
</details>File: pt-v5-vault/src/interfaces/IVaultHooks.sol 26: function beforeClaimPrize( address winner, uint8 tier, uint32 prizeIndex, uint96 reward, address rewardRecipient ) external returns (address);
uints/ints smaller than 32 bytes (256 bits) incurs overheadUsage of uints/ints smaller than 32 bytes (256 bits) incurs overhead. The Ethereum Virtual Machine (EVM) operates on 32 bytes at a time. Therefore, if an element is smaller than 32 bytes, the EVM must use more operations to reduce the size of the element from 32 bytes to the desired size.
Operations involving smaller size uints/ints cost extra gas due to the compiler having to clear the higher bits of the memory word before operating on the small size integer. This also includes the associated stack operations of doing so.
It's recommended to use larger sizes and downcast where needed to optimize for gas efficiency.
<details> <summary><i>19 issue instances in 4 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 74: uint32 public constant FEE_PRECISION = 1e9; 80: uint32 public constant MAX_YIELD_FEE = 9e8; 119: uint32 public yieldFeePercentage; 138: uint8 private immutable _underlyingDecimals; 296: uint32 yieldFeePercentage_, uint256 yieldBuffer_, address owner_ ) TwabERC20(name_, symbol_, prizePool_.twabController()) Claimable(prizePool_, claimer_) Ownable(owner_) { 304: (bool success, uint8 assetDecimals) = _tryGetAssetDecimals(asset_); 320: function decimals() public view override(ERC20, IERC20Metadata) returns (uint8) { 668: uint32 _yieldFeePercentage = yieldFeePercentage; 753: function setYieldFeePercentage(uint32 _yieldFeePercentage) external onlyOwner { 772: function _tryGetAssetDecimals(IERC20 asset_) internal view returns (bool, uint8) { 778: if (returnedDecimals <= type(uint8).max) { 779: return (true, uint8(returnedDecimals)); 800: return type(uint96).max - _totalSupply; 947: function _setYieldFeePercentage(uint32 _yieldFeePercentage) internal {
74 | 80 | 119 | 138 | 296 | 304 | 320 | 668 | 753 | 772 | 778 | 779 | 800 | 947
File: pt-v5-vault/src/PrizeVaultFactory.sol 99: uint32 _yieldFeePercentage, address _owner ) external returns (PrizeVault) {
File: pt-v5-vault/src/abstract/Claimable.sol 21: uint24 public constant HOOK_GAS = 150_000; 78: uint8 _tier, uint32 _prizeIndex, uint96 _reward, address _rewardRecipient ) external onlyClaimer returns (uint256) {
</details>File: pt-v5-vault/src/interfaces/IVaultHooks.sol 28: uint8 tier, uint32 prizeIndex, uint96 reward, address rewardRecipient ) external returns (address); 42: uint8 tier, uint32 prizeIndex, uint256 prize, address recipient ) external;
address(0)The usage of inline assembly to check if variable is the zero can save gas compared to traditional require or if statement checks.
The assembly check uses the extcodesize operation which is generally cheaper in terms of gas.
More information can be found here.
<details> <summary><i>7 issue instances in 3 files:</i></summary>File: pt-v5-vault/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();
File: pt-v5-vault/src/TwabERC20.sol 47: if (address(0) == address(twabController_)) revert TwabControllerZeroAddress();
</details>File: pt-v5-vault/src/abstract/Claimable.sol 65: if (address(prizePool_) == address(0)) revert PrizePoolZeroAddress(); 97: if (recipient == address(0)) revert ClaimRecipientZeroAddress(); 129: if (_claimer == address(0)) revert ClaimerZeroAddress();
internal/private functions only called once can be inlined to save gasinternal functions that are only called once should be inlined to save gas.
Not inlining such functions costs an extra 20 to 40 gas due to the additional JUMP instructions and stack operations required for function calls.
File: pt-v5-vault/src/PrizeVault.sol /// @audit function `_tryGetAssetDecimals()` called only once 772: function _tryGetAssetDecimals(IERC20 asset_) internal view returns (bool, uint8) {
</details>File: pt-v5-vault/src/abstract/Claimable.sol /// @audit function `_setClaimer()` called only once 128: function _setClaimer(address _claimer) internal {
immutable as private to save gasUsing private instead of public for immutables saves gas.
The compiler doesn't need to create non-payable getter functions for deployment calldata, store the bytes of the value outside of where it's used, or add another entry to the method ID table, saving 3406-3606 gas in deployment.
<details> <summary><i>4 issue instances in 3 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 112: uint256 public immutable yieldBuffer; 115: IERC4626 public immutable yieldVault;
File: pt-v5-vault/src/TwabERC20.sol 26: TwabController public immutable twabController;
</details>File: pt-v5-vault/src/abstract/Claimable.sol 24: PrizePool public immutable prizePool;
Function names for public and external methods, as well as public state variable names, can be optimized to achieve gas savings. By renaming functions to generate method IDs with two leading zero bytes, you can save 128 gas during contract deployment. Further, renaming functions for lower method IDs can conserve 22 gas per call for each sorted position shifted.
Optimizing function names for gas efficiency can result in significant savings, especially for frequently called functions or heavily deployed contracts. Reference: Solidity Gas Optimizations - Function Name
<details> <summary><i>5 issue instances in 5 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol 65: contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownable {
File: pt-v5-vault/src/PrizeVaultFactory.sol 13: contract PrizeVaultFactory {
File: pt-v5-vault/src/TwabERC20.sol 19: contract TwabERC20 is ERC20, ERC20Permit {
File: pt-v5-vault/src/abstract/Claimable.sol 13: abstract contract Claimable is HookManager, IClaimable {
</details>File: pt-v5-vault/src/abstract/HookManager.sol 9: abstract contract HookManager {
A check regarding whether the current value and the new value are the same should be added. This helps prevent unnecessary state changes and events in case the new value is the same as the current value.
<details> <summary><i>7 issue instances in 2 files:</i></summary>File: pt-v5-vault/src/PrizeVault.sol /// @audit Missing `_claimer` check before state change 736: _setClaimer(_claimer); /// @audit Missing `_liquidationPair` check before state change 744: liquidationPair = _liquidationPair; /// @audit Missing `_yieldFeePercentage` check before state change 754: _setYieldFeePercentage(_yieldFeePercentage); /// @audit Missing `_yieldFeeRecipient` check before state change 760: _setYieldFeeRecipient(_yieldFeeRecipient); /// @audit Missing `_yieldFeePercentage` check before state change 951: yieldFeePercentage = _yieldFeePercentage; /// @audit Missing `_yieldFeeRecipient` check before state change 959: yieldFeeRecipient = _yieldFeeRecipient;
736 | 744 | 754 | 760 | 951 | 959
</details>File: pt-v5-vault/src/abstract/Claimable.sol /// @audit Missing `_claimer` check before state change 130: claimer = _claimer;
Using interfaces to make external contract calls in Solidity is convenient but can be inefficient in terms of memory utilization. Each such call involves creating a new memory location to store the data being passed, thus incurring memory expansion costs.
Inline assembly allows for optimized memory usage by re-using already allocated memory spaces or using the scratch space for smaller datasets. This can result in notable gas savings, especially for contracts that make frequent external calls.
Additionally, using inline assembly enables important safety checks like verifying if the target address has code deployed to it using extcodesize(addr) before making the call, mitigating risks associated with contract interactions.
File: pt-v5-vault/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)); 539: if (_asset.allowance(_owner, address(this)) != _assets) { IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s); 639: _maxAmountOut = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this)); 854: _asset.safeTransferFrom( _caller, address(this), _assets ); 861: uint256 _assetsWithDust = _asset.balanceOf(address(this)); 862: _asset.approve(address(yieldVault), _assetsWithDust); 865: uint256 _yieldVaultShares = yieldVault.previewDeposit(_assetsWithDust); 866: uint256 _assetsUsed = yieldVault.mint(_yieldVaultShares, address(this)); 869: _asset.approve(address(yieldVault), 0); 922: return yieldVault.convertToAssets(yieldVault.maxRedeem(address(this))); 931: uint256 _latentAssets = _asset.balanceOf(address(this)); 934: uint256 _yieldVaultShares = yieldVault.previewWithdraw(_assets - _latentAssets); 936: yieldVault.redeem(_yieldVaultShares, address(this), address(this)); 939: _asset.transfer(_receiver, _assets);
337 | 382 | 383 | 405 | 416 | 539 | 639 | 854 | 861 | 862 | 865 | 866 | 869 | 922 | 931 | 934 | 936 | 939
File: pt-v5-vault/src/TwabERC20.sol 59: return twabController.balanceOf(address(this), _account); 64: return twabController.totalSupply(address(this)); 77: twabController.mint(_receiver, SafeCast.toUint96(_amount)); 88: twabController.burn(_owner, SafeCast.toUint96(_amount)); 101: twabController.transfer(_from, _to, SafeCast.toUint96(_amount));
</details>File: pt-v5-vault/src/abstract/Claimable.sol 99: uint256 prizeTotal = prizePool.claimPrize( _winner, _tier, _prizeIndex, recipient, _reward, _rewardRecipient );
#0 - raymondfam
2024-03-13T04:03:49Z
45 well elaborated generic G
#1 - c4-pre-sort
2024-03-13T04:03:54Z
raymondfam marked the issue as high quality report
#2 - c4-sponsor
2024-03-14T16:41:23Z
trmid (sponsor) confirmed
#3 - c4-judge
2024-03-18T02:55:20Z
hansfriese marked the issue as grade-a
#4 - c4-judge
2024-03-18T02:55:23Z
hansfriese marked the issue as selected for report
#5 - hansfriese
2024-03-21T04:41:56Z
Around 30 findings are known issues. Check details here