Code maturity

Follow the adopted conventions for internal and private variables

Best practices for solidity development use the _ to prefix both internal and private variables and functions.

This was addressed by Revest in commit b81a509b41524c896f8bfa75785b554496e16080.

Reliance on integer underflow reversion

In numerous places reversion by integer underflow is used as an implicit check that withdraw amounts do not exceed available funds. For example, this happens in receiveResonateOutput(...) as there is no check made on the function parameter quantity. It also happens in modifyExistingOrder as there is no check on amount. Including explicit checks on these parameters would send clear messages to protocol users under transaction failure and improve the overall user experience.

Revest indicated they may address this in the future.

Confusing variable names

In general the variables are intuitively named. However, the method getAddressForFNFT(bytes32 fnftId) in ResonateHelper takes an fnftId parameter but is in fact passed a poolId. This can create some developer confusion because the variable name fnftId is also used in Resonate to denote the ID of the FNFT. We suggest Revest change the parameter name from fnftId to poolId.

Revest indicated this has been addressed.

Clear comments

At the time of audit the Resonate project is still a work in progress. There are many comments left for other developers that take the form of questions and to-do lists. There is also a general lack of good-quality comments that would be useful for someone not intimately familiar with the code base. This applies to both the core contracts and their interfaces. For example, the following comment indicates a work in progress but also points to an unused variable:

function maxDeposit(address _account)
	public
	view
	override
	returns (uint256)
{
	_account; // TODO can acc custom logic per depositor
	VaultAPI _bestVault = yVault;
	uint256 _totalAssets = _bestVault.totalAssets();
	uint256 _depositLimit = _bestVault.depositLimit();
	if (_totalAssets >= _depositLimit) return 0;
	return _depositLimit - _totalAssets;
}

Revest has made considerable improvements to the inline documentation.

Unused resources

There are potentially unused resources in the project; for example, FullMath is never used for uint256 in Resonate.

Revest has removed the unused library - 6b1b81f6c0310297f5b6cd9a258b99e43c61b092.

Control variables and abstraction

Using values of process variables like depositedShares to indicate pool and order configurations is challenging to read. And furthermore, as we have seen in these report findings, it is error prone. Revest should consider adding another layer of abstraction to more clearly illustrate pool and order configurations.