Sublime contest - hyh's results

Handle

hyh

Vulnerability details

Impact

Full withdrawal and moving funds between strategies can lead to wrong accounting if the corresponding market has tight liquidity, which can be the case at least for AaveYield. That is, as the whole amount is required to be moved at once from Aave, both withdrawAll and switchStrategy will incorrectly account for partial withdrawal as if it was full whenever the corresponding underlying yield pool had liquidity issues.

withdrawAll will delete user entry, locking the user funds in the strategy: user will get partial withdrawal and have the corresponding accounting entry removed, while the remaining actual funds will be frozen within the system.

switchStrategy will subtract full number of shares for the _amount requested from the old strategy, while adding lesser partial number of shares for _tokensReceived to the new one with the same effect of freezing user's funds within the system.

Proof of Concept

SavingsAccount.withdrawAll https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L286

SavingsAccount.switchStrategy: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L152

When full withdrawal or strategy switch is performed it is one withdraw via unlockTokens without checking the amount received.

In the same time the withdraw can fail for example for the strategy switch if old strategy is having liquidity issues at the moment, i.e. Aave market is currently have utilization rate too high to withdraw the amount requested given current size of the lending pool.

Aave unlockTokens return is correctly not matched with amount requested: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/yield/AaveYield.sol#L217

But, for example, withdrawAll ignores the fact that some funds can remain in the strategy and deletes the use entry after one withdraw attempt: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L294 https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L312

switchStrategy removes the old entry completely: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/SavingsAccount/SavingsAccount.sol#L181

Recommended Mitigation Steps

For both withdrawAll and switchStrategy the immediate fix is to account for tokens received in both cases, which are _amount after unlockTokens for withdrawAll and _tokensReceived for switchStrategy.

More general handling of the liquidity issues ideally to be addressed architecturally, given the potential issues with liquidity availability any strategy withdrawals can be done as follows:

1. Withdraw what is possible on demand, leave the amount due as is, i.e. do not commit to completing the action in one go and notify the user the action was partial (return actual amount)
2. Save to query and repeat for the remainder funds on the next similar action (this can be separate flag triggered mode)

Handle

hyh

Vulnerability details

Impact

Funds that are acquired from a liquidator and should be sent to a lender are left with the contract instead. The funds aren't lost, but after the fact mitigation will require manual accounting and fund transfer for each CreditLine.liquidate usage.

Proof of Concept

ETH sent to CreditLine.liquidate by an external liquidator when autoLiquidation is enabled remain with the contract and aren't transferred to the lender: https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/CreditLine/CreditLine.sol#L1015

Recommended Mitigation Steps

Add transfer to a lender for ETH case:

Now:

if (_borrowAsset == address(0)) {
		uint256 _returnETH = msg.value.sub(_borrowTokens, 'Insufficient ETH to liquidate');
		if (_returnETH != 0) {
				(bool success, ) = msg.sender.call{value: _returnETH}('');
				require(success, 'Transfer fail');
		}
}

To be:

if (_borrowAsset == address(0)) {
		uint256 _returnETH = msg.value.sub(_borrowTokens, 'Insufficient ETH to liquidate');
		
		(bool success, ) = _lender.call{value: _borrowTokens}('');
		require(success, 'liquidate: Transfer failed');
		
		if (_returnETH != 0) {
				(success, ) = msg.sender.call{value: _returnETH}('');
				require(success, 'liquidate: Return transfer failed');
		}
}