Front running risks on `AssetPool`'s deposits

Severity: High
Difficulty: High

Description

The lack of front-running mitigation on AssetPool's deposits might lead a user to receive fewer tokens minted than anticipated.

When a deposit is made, the amount minted depends on the underwriterToken's total supply, and the AssetPools collateral balance:

coverage-pools/contracts/AssetPool.sol

Lines 467 to 481 in 09aa125

    
           function _deposit(address depositor, uint256 amount) internal { 
        
               require(depositor != address(this), "Self-deposit not allowed"); 
        
               rewardsPool.withdraw(); 
        
               uint256 covSupply = underwriterToken.totalSupply(); 
        
               uint256 collateralBalance = collateralToken.balanceOf(address(this)); 
        
               uint256 toMint; 
        
               if (covSupply == 0) { 
        
                   toMint = amount; 
        
               } else { 
        
                   toMint = (amount * covSupply) / collateralBalance; 
        
               } 
        
               underwriterToken.mint(depositor, toMint);

Both can vary over time. As a result, there is no guarantee that the user will receive the expected amount of underwriter tokens when he made a deposit.

Exploit Scenario

Bob made a deposit. Before its transaction is accepted, the collateral balance of the AssetPool increases. As a result, Bob's receives fewer tokens than he anticipated

Recommendations

Short term, add a minAmountToMint parameter to _deposit to enforce a lower bound of the tokens minted. The deposit call should revert, if the amount minted is lower than the value of this parameter.

Long term, carefully design deposit/withdraw functions to be robust against transaction ordering.

Underwriter tokens represent a share of the pool. Even if someone has frontrunned Bob's transaction and Bob received less underwriter tokens than he expected, these tokens still have the same value as they represent the same value from pool's TVL. The only case when Bob is unhappy is when underwriter tokens are traded and Bob is counting on, for example, arbitrating opportunities. It does not feel like a primary use for a coverage pool's underwriter tokens as they are all about representing a share of the pool. For this reason, I'd suggest adding one more function accepting the minimum amount to mint next to the original deposit and accepting the minimum amount to mint as an optional calldata parameter in receiveApproval instead of requiring all underwriters to provide this parameter if they don't care about arbitraging opportunities.

To explain the underlying underwriter token value, I listed three scenarios below.

Rewards Pool released rewards

Asset Pool collateral balance increased because Rewards Pool released rewards.
100 underwriter tokens minted 100 KEEP in the pool, Bob is depositing 10 KEEP, 5 KEEP gets released from the Rewards Pool. Bob receives (10 * 100 / 105) = 9,5238 underwriter tokens.

Bob could expect to receive 10 underwriter tokens but he received 9,5238 underwriter tokens.

9,5238 tokens represent 9,5238 / 109,5238 share of the pool, so (9,5238 / 109,5238) * 115 = 10 KEEP.

Bob's deposit gets frontrunned by Eve's deposit

Bob gets frontrunned by Eve who is depositing 20 KEEP.
Eve receives: (20 * 100 / 100) = 20 underwriter tokens
Bob receives: (10 * 120 / 120) = 10 underwriter tokens

Bob received 10 underwriter tokens as expected.

10 tokens represent 10/130 share of the pool, so (10/130) * 130 = 10 KEEP.

Asset Pool externally funded

Someone funded Asset Pool without minting underwriter tokens. The situation is the same as when Rewards Pool released rewards.

	function _deposit(address depositor, uint256 amount) internal {
	require(depositor != address(this), "Self-deposit not allowed");

	rewardsPool.withdraw();

	uint256 covSupply = underwriterToken.totalSupply();
	uint256 collateralBalance = collateralToken.balanceOf(address(this));

	uint256 toMint;
	if (covSupply == 0) {
	toMint = amount;
	} else {
	toMint = (amount * covSupply) / collateralBalance;
	}
	underwriterToken.mint(depositor, toMint);