prePO contest - Aymen0909's results

QA Report

Summary

Findings

1- Front-runable initialize function :

The initialize function inside the Collateral contract is used to initialize the contract access control and important contract parameters, but any attacker can initialize the contract before the legitimate deployer and even if the developers when deploying call immediately the initialize function, malicious agents can trace the protocol deployment transactions and insert their own transaction between them and by doing so they front run the developers call and gain the ownership of the contract and set the wrong parameters.

The impact of this issue is :

• In the best case developers notice the problem and have to redeploy the contract and thus costing more gas.

• In the worst case the protocol continue to work with the wrong owner and state parameters which could lead to the loss of user funds.

Risk : Low

Proof of Concept

Instances include:

File: apps/smart-contracts/core/contracts/Collateral.sol

function initialize(string memory _name, string memory _symbol) public initializer

Mitigation

It's recommended to use the constructor to initialize non-proxied contracts.

For initializing proxy (upgradable) contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify that the transaction succeeded.

2- Manager can drain funds from Collateral contract :

The managerWithdraw function inside the Collateral contract is used to allow the manager to transfer to him self a certain amount of baseToken but it can be used to drain all the contract baseToken balance if for example the manager and withdrawal_manager were malicious, and in the case the users who deposited tokens will not be able to get them back.

Risk : Low

Proof of Concept

The issue occurs in the function below :

File: apps/smart-contracts/core/contracts/Collateral.sol Line 80-83

function managerWithdraw(uint256 _amount) external override onlyRole(MANAGER_WITHDRAW_ROLE) nonReentrant {
    if (address(managerWithdrawHook) != address(0)) managerWithdrawHook.hook(msg.sender, _amount, _amount);
    baseToken.transfer(manager, _amount);
}

If the withdrawal manager and the manager are both malicious agent they can use this function to transfer any amount of baseToken to the manager account.

Mitigation

It's recommended to use a DAO governance to control funds withdrawal from the Collateral contract in case they were needed.

3- Setters should check the input value and revert if it's the zero address or zero :

Risk : Low

When setting a new value to a state variable the setter function must check the new value and revert if it's the zero address or zero. This issue is present in almost all the setters functions a cross the different contracts

Mitigation

Add non-zero address/uint checks for the setters functions.

4- Use scientific notation :

When using multiples of 10 you shouldn't use decimal literals or exponentiation (e.g. 1000000, 10**18) but use scientific notation for better readability.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: apps/smart-contracts/core/contracts/Collateral.sol Line 19-20

uint256 public constant FEE_DENOMINATOR = 1000000;
uint256 public constant FEE_LIMIT = 100000;

File: apps/smart-contracts/core/contracts/DepositTradeHelper.sol Line 12

uint24 public constant override POOL_FEE_TIER = 10000;

File: apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol Line 12

uint256 public constant PERCENT_DENOMINATOR = 1000000;

File: apps/smart-contracts/core/contracts/PrePOMarket.sol Line 30-31

uint256 private constant FEE_DENOMINATOR = 1000000;
uint256 private constant FEE_LIMIT = 100000;

File: apps/smart-contracts/core/contracts/TokenSender.sol Line 25

uint256 public constant MULTIPLIER_DENOMINATOR = 10000;

Gas Optimizations

Summary

Findings

1- Remove unecessary lines of code :

In the redeem function inside the PrePOMarket contract, the following lines of code are unecessary and should be removed to save gas :

File: apps/smart-contracts/core/contracts/PrePOMarket.sol Line 95

uint256 _collateralAllowanceBefore = collateral.allowance(address(this), address(_redeemHook));

File: apps/smart-contracts/core/contracts/PrePOMarket.sol Line 97

_actualFee = _collateralAllowanceBefore - collateral.allowance(address(this), address(_redeemHook));

The above lines are used to calculated the _actualFee but following the code logic it's clear the we always have _actualFee == _expectedFee, this is the case because the _redeemHook.hook function is passed the following parameters :

amountBeforeFee = _collateralAmount
amountAfterFee = _collateralAmount - _expectedFee

And thus the fee reduced from the allowance balance would be :

fee = amountBeforeFee - amountAfterFee = _collateralAmount - (_collateralAmount - _expectedFee)
fee = _expectedFee

Meaning that the allowanceBefore will be decreased by _expectedFee which is exactly equal to _actualFee calculted with the formula :

_actualFee = _collateralAllowanceBefore - collateral.allowance(address(this), address(_redeemHook));

The _expectedFee value should be used directly and those lines of code should be removed to save gas.

2- Use unchecked blocks to save gas :

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

There are 10 instances of this issue:

File: apps/smart-contracts/core/contracts/Collateral.sol Line 50

uint256 _amountAfterFee = _amount - _fee;

The above operation cannot underflow due to the line :

uint256 _fee = (_amount * depositFee) / FEE_DENOMINATOR;

Because depositFee is always less than FEE_DENOMINATOR we always have the following _amount > _fee and thus the operation should be marked unchecked

File: apps/smart-contracts/core/contracts/Collateral.sol Line 70

uint256 _baseTokenAmountAfterFee = _baseTokenAmount - _fee;

The above operation cannot underflow due to the line :

uint256 _fee = (_baseTokenAmount * withdrawFee) / FEE_DENOMINATOR;

Because withdrawFee is always less than FEE_DENOMINATOR we always have the following _baseTokenAmount > _fee and thus the operation should be marked unchecked

File: apps/smart-contracts/core/contracts/DepositHook.solLine 47

uint256 _fee = _amountBeforeFee - _amountAfterFee;

The above operation cannot underflow because only collateral contract can call the hook function with deposit function which ensure that we always have _amountBeforeFee > _amountAfterFee as we have _amount=_amountBeforeFee > _amountAfterFee=_amountAfterFee, and thus the operation should be marked unchecked

File: apps/smart-contracts/core/contracts/WithdrawHook.sol Line 74

uint256 _fee = _amountBeforeFee - _amountAfterFee;

The above operation cannot underflow because only collateral contract can call the hook function with withdraw function which ensure that we always have _amountBeforeFee > _amountAfterFee as we have _baseTokenAmount=_amountBeforeFee > _baseTokenAmountAfterFee=_amountAfterFee, and thus the operation should be marked unchecked

File: apps/smart-contracts/core/contracts/DepositRecord.sol Line 36

globalNetDepositAmount -= _amount;

The above operation cannot underflow due to the check if (globalNetDepositAmount > _amount) and should be marked unchecked

File: apps/smart-contracts/core/contracts/DepositRecord.sol Line 82

uint256 _shortPayout = MAX_PAYOUT - finalLongPayout;

The above operation cannot underflow due to the check if (finalLongPayout <= MAX_PAYOUT) and should be marked unchecked

File: apps/smart-contracts/core/contracts/WithdrawHook.sol Line 64

globalAmountWithdrawnThisPeriod += _amountBeforeFee;

The above operation cannot overflow due to the check :

require(globalAmountWithdrawnThisPeriod + _amountBeforeFee <= globalWithdrawLimitPerPeriod, "global withdraw limit exceeded");

This check ensures that globalAmountWithdrawnThisPeriod + _amountBeforeFee is always less than globalWithdrawLimitPerPeriod and thus the operation can't overflow and should be marked unchecked

File: apps/smart-contracts/core/contracts/WithdrawHook.sol Line 71

userToAmountWithdrawnThisPeriod[_sender] += _amountBeforeFee;

The above operation cannot overflow due to the check :

require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");

This check ensures that userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee is always less than userWithdrawLimitPerPeriod and thus the operation can't overflow and should be marked unchecked

File: apps/smart-contracts/core/contracts/DepositRecord.sol Line 31

globalNetDepositAmount += _amount;

The above operation cannot overflow due to the check :

require(_amount + globalNetDepositAmount <= globalNetDepositCap, "Global deposit cap exceeded");

This check ensures that _amount + globalNetDepositAmount is always less than globalNetDepositCap and thus the operation can't overflow and should be marked unchecked

File: apps/smart-contracts/core/contracts/DepositRecord.sol Line 32

userToDeposits[_sender] += _amount;

The above operation cannot overflow due to the check :

require(_amount + userToDeposits[_sender] <= userDepositCap, "User deposit cap exceeded");

This check ensures that _amount + userToDeposits[_sender] is always less than userDepositCap and thus the operation can't overflow and should be marked unchecked

3- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 5 instances of this issue:

File: apps/smart-contracts/core/contracts/DepositRecord.sol Line 31-32

globalNetDepositAmount += _amount;
userToDeposits[_sender] += _amount;

File: apps/smart-contracts/core/contracts/DepositRecord.sol Line 36

globalNetDepositAmount -= _amount;

File: apps/smart-contracts/core/contracts/WithdrawHook.sol Line 64

globalAmountWithdrawnThisPeriod += _amountBeforeFee;

File: apps/smart-contracts/core/contracts/WithdrawHook.sol Line 71

userToAmountWithdrawnThisPeriod[_sender] += _amountBeforeFee;