prePO contest - 0xDjango's results

Lines of code

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

Vulnerability details

Impact

A bad actor can steal funds from future depositors by sending the base token directly to the Strategy or StrategyController contracts. This exploit is more effective the less shares that have already been distributed, perhaps early into the launch of the protocol, preferably the first to deposit.

Proof of Concept

Note: I will be using proper token decimals for all examples.

The exploit occurs in the Collateral.sol contract in the deposit function. https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/Collateral.sol#L81-L91

For the first user to deposit the base token (USDC), they receive an equal number of PreCT. Let's assume Bad Actor A is the first to deposit 2e0 USDC. Note that this is is 000002 USDC. This value was specifically chosen because 000001 USDC will be taken for the fee based on the following formula:

uint256 _fee = (_amountToDeposit * _mintingFee) / FEE_DENOMINATOR + 1;

Assuming that the _minting fee is less than the FEE_DENOMINATOR, this will return 1. This leaves 1e0 USDC to be deposited in exchange for preCT. The bad actor receives 1e0 PreCT share in return, while totalSupply() = 0.

User Shares: Bad Actor A: 1e0

Then the bad actor sends 999999999 USDC (roughly $1000) directly to either the StrategyController or Strategy contracts. For subsequent user deposits, the calculation to distribute shares will be wildly skewed based on the share calculation formula:

_shares = (_amountToDeposit * totalSupply()) / (_valueBefore);

Where totalSupply() is equal to the total number of PreCT shares minted and _valueBefore is equal to _strategyController.totalValue().

_strategyController.totalValue() resolves to _baseToken.balanceOf(address(this)) + _strategy.totalValue(); which calculates to the amount of base token in the StrategyController and Strategy contracts + the estimated value of deployed assets.

For our example, Good User B attempts to deposit $500 in USDC. The shares that Good User B will receive looks as such (not taking into account deposit fees):

_shares = (500e6 USDC * 1e0 Minted PreCT) / (1000e6 USDC in Strategy Contracts); = 0 Shares due to how solidity performs division.

Unfortunately, the transfers from the user to the Collateral contract to the StrategyController contracts have already taken place. Finally, the _mint() function is called with 0 shares, effectively giving Good User B nothing in return.

User Shares: Bad Actor A: 1 Good User B: 0

Now, Bad Actor A can initiate a withdrawal (pertaining to the rules of the delayed withdrawal procedures). The withdrawal amount calculation is the inverse of the shares to mint formula:

uint256 _owed = (_strategyController.totalValue() * _amount) / totalSupply();

Our scenario:

_shares = (1500e6 USDC * 1e0 correctly deposited) / (1e0 Minted PreCT); = $1500 which is all of the bad actor's transferred funds and the funds transferred from Good User B.

This concludes the main attack vector that Bad Actor A would follow. Some other considerations are listed below:

• This exploit will work best if there are many deposits after the start of the exploit and 0 organic withdrawals during the window of the exploit. Due to the nature of the withdrawal initiation pattern, the exploiter can monitor the contract to understand other users' intents to withdraw.
• Another negative effect of this exploit is that it makes the "buy in" to receive a single PreCT much higher than expected. In the above example, a good user will need to invest at least $1000 for a single PreCT, meaning that participation in the protocol's actual functionality will be highly limited.

Recommended Mitigation Steps

There would need to be internal accounting variables to indicate fund movement from all contracts.

• One to indicate the funds that flowed from Collateral.sol to StrategyController.sol
• One to indicate the funds that flowed from StrategyController.sol to Strategy.sol
• One to indicate funds deployed.
• One to indicate funds withdrawn from strategy deployment (for good measure).

Any discrepancy in the first 3 variables might indicate direct fund transfers. Use the minimum value of these three as totalValue()

The accounting variables should then be used in the original deposit and withdrawal logic in Collateral.sol, while the Bad Actor's extra funds can still be used for yield generation.

Also, another possible "easy solution" to this problem would be to seed the collateral vault with an initial deposit. The more shares that have been minted prior to exploit, the less beneficial this would be for the attacker. The issue here is that the initial deposit could potentially be frontrun.

Issue: _delayedWithrawExpiry time can be arbitrarily set despite previously initiated withdrawals (low severity)

Link to Code

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

and

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

Impact

Imagine a user initiates a withdrawal understanding that the current _delayedWithdrawExpiry = 100 blocks. Then, the _delayedWithrawExpiry is decided to be better set at 50. The user might try to withdraw after 75 blocks but the transaction will revert.

Steps to remediate

Similar to setting the amount and blockNumber in the initateWithdraw function, also set the delayedWithdrawExpiry for the account:

_accountToWithdrawalRequest[msg.sender].amount = _amount;
_accountToWithdrawalRequest[msg.sender].blockNumber = block.number;
_accountToWithdrawalRequest[msg.sender].delayedWithdrawExpiry = getDelayedWithdrawalExpiry();

Then check this value in the _processDelayedWithdrawal function instead of the current _delayedWithdrawExpiry.