prePO contest - CertoraInc's results

Lines of code

https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/Collateral.sol#L81-L91 https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/SingleStrategyController.sol#L32-L40 https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/SingleStrategyController.sol#L79-L81

Vulnerability details

Impact

That exploit is possible because of the implementation of the deposit function of the SingleStrategyController contract.

// Assumes approval to take `_amount` has already been given by vault
function deposit(uint256 _amount)
    external
    override
    onlyVault
    nonReentrant
{
    _baseToken.safeTransferFrom(_vault, address(this), _amount);
    _strategy.deposit(_baseToken.balanceOf(address(this)));
}

If an attacker manages to be the first user who deposits to the Collateral contract, he'll get his shares minted (an amount that is equal to the amount of tokens he provided). Let's assume he deposited the minimum amount of token which is 2, so he'll get 1 share (because the fees will be 1). Then, he'll front-run every call to deposit function of the Collateral contract and transfer amount of _baseToken to the SingleStrategyController so that the totalValue() will be greater than the amount of tokens the user wants to deposit. So now, when the user's deposit will be executed, he'll transfer the tokens, pay the fees and deposit it to the strategy (which will also deposit the tokens that the attacker had transferred before). But the user won't get any shares minted! That's because _shares = (_amountToDeposit * totalSupply()) / (_valueBefore); will assign 0 to _shares, because the attacker made sure that _valueBefore (which is the value of totalValue() before the deposit) is greater than _amountToDeposit, and we know that totalSupply() == 1. So the user won't get any shares minted, but the tokens will be transferred and deposited to the strategy, and the attacker (which has all the shares) will be able to withdraw all these tokens from the strategy - by simply call withdraw with _amount = 1. That will give him uint256 _owed = (_strategyController.totalValue() * _amount) / totalSupply(); tokens, which is exactly totalValue(). Of course that attacker will have to pay fees from this withdraw, but it's still profitable for him.

Tools Used

Remix and VS Code

Recommended Mitigation Steps

This can be fixed by changing the implementation of SingleStrategyController - by changing the deposit function to deposit exactly the amount of token recieved, or by changing the totalValue function to return only the value in the strategy.

Lows and Non-Critical

check that the deposit cap is greater than the deposit amount in setGlobalDepositCap in the CollateralDepositRecord contract

This might be a thing you want to check, to avoid from a situation where the system is in an invalid state.

inconsistent usage of safe and unsafe transfer functions

Use safeTransfer and safeTransferFrom instead of using transfer and transferFrom in the PrePOMarket contract.

wrong comment in the deposit function of the Collateral contract

cost per share is total value / total supply, and not total supply / total value (the implementation is correct, just the comment is wrong).

/**
* # of shares owed = amount deposited / cost per share, cost per
* share = total supply / total value.
*/

front-runnable initializer (can be solved using access controls)

An attacker can front run and call the initialize function with unwanted values and taking over the ownership of the contract, making the contract to be redeployed. This can be avoided by giving access controls in the constructor, which means that only the address that created the contract will be able to call the initialize function.

LongShortToken contract doesn't implement the corresponding ILongShortToken interface (it does implement its functions, just not declaring that it is implementing it)

Gas Optimizations

avoid transferring the fees in every function call

Instead of transferring the fees in every function call, the fees amount can be saved in a variable, and you can create a function that transfers the fees and zeros that variable. That will save the gas spent on the transfer that is being executed in every function call, and will make the transfer function being used (with the fees) much less than the current implementation.

This optimization can be done both in the PrePOMarket and the Collateral contracts (the Collateral contract will need some extra changes, for example to subtract the fees amount in the totalAssets function or in the amount deposited calculation in the deposit function).

loops in allowAccounts and blockAccounts functions in AccountAccessController

The loops in the allowAccounts and blockAccounts functions in the AccountAccessController contract can be optimized using several optimizations:

1. Variables in solidity are already initialized to their default value, and initializing them to the same value actually costs more gas. So the code can be optimized using uint i; instead of uint i = 0;.
2. Use ++i instead of i++ to save some gas spent in every iteration (this is cheaper and does the same in this case).
3. Use unchecked on the loop index incrementing.
4. Save the array length in a local variable before the loop instead of accessing it in every iteration.
5. Save the _accounts[_i] variables instead of accessing it twice.
6. Save the _allowedAccountsIndex or _blockedAccountsIndex storage variable on the stack to save gas.

So after changing the code using the optimizations I mentioned before, it will look something like this:

old code:

function allowAccounts(address[] calldata _accounts)
    external
    override
    onlyOwner
{
    for (uint256 _i = 0; _i < _accounts.length; _i++) {
        _allowedAccounts[_allowedAccountsIndex][_accounts[_i]] = true;
        emit AccountAllowed(_accounts[_i]);
    }
}

function blockAccounts(address[] calldata _accounts)
    external
    override
    onlyOwner
{
    for (uint256 _i = 0; _i < _accounts.length; _i++) {
        _blockedAccounts[_blockedAccountsIndex][_accounts[_i]] = true;
        emit AccountBlocked(_accounts[_i]);
    }
}

new code:

function allowAccounts(address[] calldata _accounts)
    external
    override
    onlyOwner
{
    address account_i;
    uint length = _accounts.length;
    uint index = _allowedAccountsIndex;
    for (uint256 _i; _i < length;) {
        account_i = _accounts[_i];
        _allowedAccounts[index][account_i] = true;
        emit AccountAllowed(account_i);
        unchecked { ++_i; }
    }
}

function blockAccounts(address[] calldata _accounts)
    external
    override
    onlyOwner
{
    address account_i;
    uint length = _accounts.length;
    uint index = _blockedAccountsIndex;
    for (uint256 _i; _i < length;) {
        account_i = _accounts[_i];
        _blockedAccounts[index][account_i] = true;
        emit _blockedAccounts(account_i);
        unchecked { ++_i; }
    }
}

Inline some functions

Some short (and mostly internal) functions can be inlined in order to save the gas spent while calling them. For example, the _setRoot and _clearAllowedAccounts functions in AccountAccessController can be inlined. This is their code:

function _setRoot(bytes32 _newRoot) internal {
    _root = _newRoot;
    emit RootChanged(_root);
}

function _clearAllowedAccounts() internal {
    _allowedAccountsIndex++;
    emit AllowedAccountsCleared(_allowedAccountsIndex);
}

They are internal, so anywhere they are being used you can write their code instead to save gas.

Another example is the call to totalAssets in the getSharesForAmount function in Collateral contract. In this code we see the call to totalAssets, but this is an internal function that returns _baseToken.balanceOf(address(this)) + _strategyController.totalValue(), which can be calculated directly without calling the function.

function getSharesForAmount(uint256 _amount)
    external
    view
    override
    returns (uint256)
{
    uint256 _totalAssets = totalAssets();
    return
        (_totalAssets > 0)
            ? ((_amount * totalSupply()) / _totalAssets)
            : 0;
}

use prefix inc instead of postfix inc in clearBlockedAccounts and _clearAllowedAccounts functions in AccountAccessController

++variable is cheaper than variable++, and in these cases it does the same. It can even be simplified more.

old code:

function _clearAllowedAccounts() internal {
    _allowedAccountsIndex++;
    emit AllowedAccountsCleared(_allowedAccountsIndex);
}

function clearBlockedAccounts() external override onlyOwner {
    _blockedAccountsIndex++;
    emit BlockedAccountsCleared(_blockedAccountsIndex);
}

new code:

function _clearAllowedAccounts() internal {
    emit AllowedAccountsCleared(++_allowedAccountsIndex);
}

function clearBlockedAccounts() external override onlyOwner {
    emit BlockedAccountsCleared(++_blockedAccountsIndex);
}

use unchecked to save gas

Unchecked can be used in order to disable arithmetic overflow and underflow checks, which saves some gas. This can be done in cases where we know for sure that the expressions won't overflow/underflow. For example, in the recordWithdrawal function in the CollateralDepositRecord contract, unchecked can be used when calculating the expressions, because the if statement assures that they won't underflow:

function recordWithdrawal(address _sender, uint256 _amount)
    external
    override
    onlyAllowedHooks
{
    if (_globalDepositAmount > _amount) {
        unchecked {
            _globalDepositAmount -= _amount;
        }
    } else {
        _globalDepositAmount = 0;
    }
    if (_accountToNetDeposit[_sender] > _amount) {
        unchecked {
            _accountToNetDeposit[_sender] -= _amount;
        }
    } else {
        _accountToNetDeposit[_sender] = 0;
    }
}

Another example is in the fee calculation in the withdraw and deposit functions of the Collateral contract. The fee calculation looks like this:

uint256 _fee = (_amountToDeposit * _mintingFee) / FEE_DENOMINATOR + 1;
require(_amountToDeposit > _fee, "Deposit amount too small");
_baseToken.safeTransfer(_treasury, _fee);
_amountToDeposit -= _fee;

The require assures that the expression _amountToDeposit -= _fee; won't underflow, so we can use unchecked on it too:

uint256 _fee = (_amountToDeposit * _mintingFee) / FEE_DENOMINATOR + 1;
require(_amountToDeposit > _fee, "Deposit amount too small");
_baseToken.safeTransfer(_treasury, _fee);
unchecked { _amountToDeposit -= _fee };

This can also be done in the mintLongShortTokens and redeem functions of the PrePOMarket contract in the fees calculation, and in this line in the redeem function: uint256 _shortPrice = MAX_PRICE - _finalLongPrice;.

save return value instead of calling a function multiple times

The deposit function in Collateral calls totalSupply() twice, where there are no changes made to the totalSupply between the calls, which means it will return the same value. So instead of calling it twice, this return value can be saved and used again.

old code:

...
if (totalSupply() == 0) {
    _shares = _amountToDeposit;
} else {
    /**
     * # of shares owed = amount deposited / cost per share, cost per
     * share = total supply / total value.
     */
    _shares = (_amountToDeposit * totalSupply()) / (_valueBefore);
}
...

new code:

...
uint _totalSupply = totalSupply();
if (_totalSupply == 0) {
    _shares = _amountToDeposit;
} else {
    /**
     * # of shares owed = amount deposited / cost per share, cost per
     * share = total supply / total value.
     */
    _shares = (_amountToDeposit * _totalSupply) / (_valueBefore);
}
...

save values of storage variables in recordDeposit function in CollateralDepositRecord

The _accountToNetDeposit[_sender] and _globalDepositAmount storage variables are accessed multiple times, and can be saved on the stack as local variables to save gas. In addition, the calculation can be calculated once and saved instead of being calculated twice.

old code:

function recordDeposit(address _sender, uint256 _amount)
    external
    override
    onlyAllowedHooks
{
    require(
        _amount + _globalDepositAmount <= _globalDepositCap,
        "Global deposit cap exceeded"
    );
    require(
        _amount + _accountToNetDeposit[_sender] <= _accountDepositCap,
        "Account deposit cap exceeded"
    );
    _globalDepositAmount += _amount;
    _accountToNetDeposit[_sender] += _amount;
}

new code:

function recordDeposit(address _sender, uint256 _amount)
    external
    override
    onlyAllowedHooks
{
    uint256 depositAmount = _globalDepositAmount + _amount;
    uint256 depositToAccountAmount = _accountToNetDeposit[_sender] + _amount;
    require(
        depositAmount <= _globalDepositCap,
        "Global deposit cap exceeded"
    );
    require(
        depositToAccountAmount <= _accountDepositCap,
        "Account deposit cap exceeded"
    );
    _globalDepositAmount = depositAmount;
    _accountToNetDeposit[_sender] = depositToAccountAmount;
}

redundant initialization in deposit function

In solidity, variables are initialized to their default value, so uint256 variables will be automatically initialized to 0, and doing it manually just costs more gas - uint256 x; is cheaper than uint256 x = 0;, although it does the same.

https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/Collateral.sol#L81

checking if totalSupply() > 0 consumes less gas than checking if totalSupply() == 0

old code:

if (totalSupply() == 0) {
    _shares = _amountToDeposit;
} else {
    /**
     * # of shares owed = amount deposited / cost per share, cost per
     * share = total supply / total value.
     */
    _shares = (_amountToDeposit * totalSupply()) / (_valueBefore);
}

new code:

if (totalSupply() > 0) {
    /**
     * # of shares owed = amount deposited / cost per share, cost per
     * share = total supply / total value.
     */
    _shares = (_amountToDeposit * totalSupply()) / (_valueBefore);
} else {
    _shares = _amountToDeposit;
}

no need for the _balanceBefore and _balanceAfter variables in the withdraw function

These variables can be replaced with only once variable. That will look something like that:

old code:

...
uint256 _balanceBefore = _baseToken.balanceOf(address(this));
_strategyController.withdraw(address(this), _owed);
uint256 _balanceAfter = _baseToken.balanceOf(address(this));

uint256 _amountWithdrawn = _balanceAfter - _balanceBefore;
...

new code:

...
uint256 _amountWithdrawn = _baseToken.balanceOf(address(this));
_strategyController.withdraw(address(this), _owed);
_amountWithdrawn = _baseToken.balanceOf(address(this)) - _amountWithdrawn;
...