C4-001 :Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom
Impact - LOW
Impact
It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.
Consider using safeTransfer/safeTransferFrom or require() consistently.
C4-002 : Use of Block.timestamp
Impact - Non-Critical
Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.
Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.
Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.
C4-003 : Missing events for only functions that change critical parameters
Impact - Non critical
The afunctions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.
Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.
Add events to all functions that change critical parameters.
C4-004 : # DoS With Block Gas Limit
Impact - Non-Critical
When smart contracts are deployed or functions inside them are called, the execution of these actions always requires a certain amount of gas, based of how much computation is needed to complete them. The Ethereum network specifies a block gas limit and the sum of all transactions included in a block can not exceed the threshold.
Programming patterns that are harmless in centralized applications can lead to Denial of Service conditions in smart contracts when the cost of executing a function exceeds the block gas limit. Modifying an array of unknown size, that increases in size over time, can lead to such a Denial of Service condition.
Proof of Concept
Follow the functions shown below.
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::185 => require(protocols.length == handlers.length, Error.INVALID_ARGUMENT);
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::188 => for (uint256 i = 0; i < protocols.length; i++) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::455 => uint256 length = userRecordsMeta.length;
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::456 => for (uint256 i = 0; i < length; i++) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::477 => uint256 length = _supportedProtocols.length();
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::478 => bytes32[] memory protocols = new bytes32[](length);
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::479 => for (uint256 i = 0; i < length; i++) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::499 => uint256 length = _usersWithPositions.length();
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::500 => if (cursor >= length) return (new address[](0), 0);
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::501 => if (howMany >= length - cursor) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::502 => howMany = length - cursor;
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::878 => if (_userPositions[payer].length == 0) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::890 => uint256 length = positionsMeta.length;
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::891 => for (uint256 i = 0; i < length; i++) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol::894 => positionsMeta[i] = positionsMeta[length - 1];
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpKeeperHelper.sol::42 => if (users.length == 0) return (_shortenTopups(executableTopups, topupsAdded), 0);
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpKeeperHelper.sol::43 => for (uint256 i = 0; i < users.length; i++) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpKeeperHelper.sol::46 => for (uint256 j = 0; j < positions.length; j++) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpKeeperHelper.sol::51 => uint256 offset = j == positions.length - 1 ? 1 : 0;
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpKeeperHelper.sol::71 => bool[] memory results = new bool[](keys.length);
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpKeeperHelper.sol::72 => for (uint256 i = 0; i < keys.length; i++) {
2022-04-backd-c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpKeeperHelper.sol::91 => uint256 length = userRecordsMeta.length;
Tools Used
Code Review
Recommended Mitigation Steps
Caution is advised when you expect to have large arrays that grow over time. Actions that require looping across the entire data structure should be avoided.
If you absolutely must loop over an array of unknown size, then you should plan for it to potentially take multiple blocks, and therefore require multiple transactions.
C4-005 : Critical changes should use two-step procedure
Impact - NON CRITICAL
The critical procedures should be two step process.
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
C4-006 : Front-runnable Initializers
Impact - LOW
All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.
initialize functions does not have access control. They are vulnerable to front-running.
Tools Used
Manual Code Review
Recommended Mitigation Steps
While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.
C4-007 : Incompatibility With Rebasing/Deflationary/Inflationary tokens
Impact - LOW
PrePo protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.
Ensure that to check previous balance/after balance equals to amount for any rebasing/inflation/deflation
Add support in contracts for such tokens before accepting user-supplied tokens
Consider supporting deflationary / rebasing / etc tokens by extra checking the balances before/after or strictly inform your users not to use such tokens if they don't want to lose them.
C4-008 : # Pragma Version
Impact
In the contracts, floating pragmas should not be used. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
Consider adding zero-address checks in the discussed constructors:
require(newAddr != address(0));.
C4-010 : # Lack of setter function for the oracle
Impact
On the UniswapV2PathPriceOracle contract, there is not setter function on the oracles addresses. This can cause misfunctionality on the uniswap oracle contract.
Setter function is missing on the contract. Misdeployed contract can cause failure of oracle integration.
Tools Used
None
Recommended Mitigation Steps
Consider to add setter function for oracles addresses.
C4-0011 : The Contract Should approve(0) first
Impact - LOW
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value.
They must first be approved by zero and then the actual allowance must be approved.
When trying to re-approve an already approved token, all transactions revert and the protocol cannot be used.
Tools Used
None
Recommended Mitigation Steps
Approve with a zero amount first before setting the actual amount. Consider use safeIncreaseAllowance and safeDecreaseAllowance.
C4-0012 : The Contract Should approve(0) first
Description
The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction.
Avoid the use of transfer() and send() and do not otherwise specify a fixed amount of gas when performing calls. Use .call.value(...)("") instead. Use the checks-effects-interactions pattern and/or reentrancy locks to prevent reentrancy attacks.
C4-0013 : # Missing Re-entrancy Guard
Impact - LOW
The re-entrancy guard is missing on the some of the functions. The external interactions can cause to the re-entrancy vulnerability.
C4-001 :Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom
Impact - LOW
Impact
It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.
Reference: This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call
Proof of Concept
Navigate to the following contract.
transfer/transferFrom functions are used instead of safe transfer/transferFrom on the following contracts.
Tools Used
Code Review
Recommended Mitigation Steps
Consider using safeTransfer/safeTransferFrom or require() consistently.
C4-002 : Use of Block.timestamp
Impact - Non-Critical
Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.
Proof of Concept
Tools Used
Manual Code Review
Recommended Mitigation Steps
Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.
Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.
C4-003 : Missing events for only functions that change critical parameters
Impact - Non critical
The afunctions that change critical parameters should emit events. Events allow capturing the changed parameters so that off-chain tools/interfaces can register such changes with timelocks that allow users to evaluate them and consider if they would like to engage/exit based on how they perceive the changes as affecting the trustworthiness of the protocol or profitability of the implemented financial services. The alternative of directly querying on-chain contract state for such changes is not considered practical for most users/usages.
Missing events and timelocks do not promote transparency and if such changes immediately affect users’ perception of fairness or trustworthiness, they could exit the protocol causing a reduction in liquidity which could negatively impact protocol TVL and reputation.
Proof of Concept
See similar High-severity H03 finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity M01 finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)
Tools Used
None
Recommended Mitigation Steps
Add events to all functions that change critical parameters.
C4-004 : # DoS With Block Gas Limit
Impact - Non-Critical
When smart contracts are deployed or functions inside them are called, the execution of these actions always requires a certain amount of gas, based of how much computation is needed to complete them. The Ethereum network specifies a block gas limit and the sum of all transactions included in a block can not exceed the threshold.
Programming patterns that are harmless in centralized applications can lead to Denial of Service conditions in smart contracts when the cost of executing a function exceeds the block gas limit. Modifying an array of unknown size, that increases in size over time, can lead to such a Denial of Service condition.
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
Caution is advised when you expect to have large arrays that grow over time. Actions that require looping across the entire data structure should be avoided.
If you absolutely must loop over an array of unknown size, then you should plan for it to potentially take multiple blocks, and therefore require multiple transactions.
C4-005 : Critical changes should use two-step procedure
Impact - NON CRITICAL
The critical procedures should be two step process.
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
C4-006 : Front-runnable Initializers
Impact - LOW
All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.
Proof of Concept
Tools Used
Manual Code Review
Recommended Mitigation Steps
While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the
msg.senderand adding theonlyOwnermodifier to all initializers would be a sufficient level of access control.C4-007 : Incompatibility With Rebasing/Deflationary/Inflationary tokens
Impact - LOW
PrePo protocol do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.
Proof of Concept
Tools Used
Manual Code Review
Recommended Mitigation Steps
C4-008 : # Pragma Version
Impact
In the contracts, floating pragmas should not be used. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
Proof of Concept
https://swcregistry.io/docs/SWC-103
Tools Used
Manual code review
Recommended Mitigation Steps
Lock the pragma version: delete pragma solidity 0.8.10 in favor of pragma solidity 0.8.10
C4-009 : # Missing zero-address check in the setter functions and initiliazers
Impact
Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are updated incorrectly.
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
Consider adding zero-address checks in the discussed constructors: require(newAddr != address(0));.
C4-010 : # Lack of setter function for the oracle
Impact
On the UniswapV2PathPriceOracle contract, there is not setter function on the oracles addresses. This can cause misfunctionality on the uniswap oracle contract.
Proof of Concept
Tools Used
None
Recommended Mitigation Steps
Consider to add setter function for oracles addresses.
C4-0011 : The Contract Should approve(0) first
Impact - LOW
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
Proof of Concept
Tools Used
None
Recommended Mitigation Steps
Approve with a zero amount first before setting the actual amount. Consider use safeIncreaseAllowance and safeDecreaseAllowance.
C4-0012 : The Contract Should approve(0) first
Description
The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction.
Proof of Concept
Remediation
Avoid the use of transfer() and send() and do not otherwise specify a fixed amount of gas when performing calls. Use .call.value(...)("") instead. Use the checks-effects-interactions pattern and/or reentrancy locks to prevent reentrancy attacks.
C4-0013 : # Missing Re-entrancy Guard
Impact - LOW
The re-entrancy guard is missing on the some of the functions. The external interactions can cause to the re-entrancy vulnerability.
Proof of Concept
Tools Used
Code Review
Recommended Mitigation Steps
Follow the check effect interaction pattern or put re-entrancy guard.