VendorPoolFactory Manual Review Findings

VPF-01M: Inexistent Prevention of Equal Asset Pool Creation

Type	Severity	Location
Logical Fault		VendorPoolFactory.sol:L134-L143

Description:

The deployPool function does not prevent the same asset from being listed as collateral and as the lending token, which will lead to the pool created significantly misbehaving in terms of tokenomics as well as functionality of the system in general.

Example:

Copy
134function deployPool(
135    uint256 _mintRatio,
136    address _colToken,
137    address _lendToken,
138    uint48 _feeRate,
139    uint256 _type,
140    uint48 _expiry,
141    address[] calldata _borrowers,
142    uint256 _undercollateralized
143) external whenNotPaused returns (address) {
144    if (!allowList[_lendToken]) revert LendTokenNotSupported();
145    if (!allowList[_colToken]) revert ColTokenNotSupported();
146    if (_mintRatio == 0) revert MintRatio0();
147    if (_expiry < block.timestamp + 24 hours) revert InvalidExpiry();
148    if (_feeRate > 1000000) revert FeeTooLarge();
149    if (_undercollateralized < 0 || _undercollateralized > 1) revert InvalidType();
150    
151    return _initializePool(
152        UserPoolData(
153            _mintRatio,
154            _colToken,
155            _lendToken,
156            _feeRate,
157            _type,
158            _expiry,
159            _borrowers,
160            _undercollateralized
161        )
162    );
163}

Recommendation:

We advise an equality check to be performed preventing the same asset from being set as the collateral and lending token of the pool.

Alleviation:

An additional sanitization step was introduced ensuring that the _colToken is not equal to the _lendToken thus addressing this exhibit in full and preventing same token pools from being created.

VPF-02M: Ill-Advised Toggle Pattern

Type	Severity	Location
Language Specific		VendorPoolFactory.sol:L337-L340, L343-L346, L349-L352

Description:

The linked functions conform to a "toggle" pattern whereby a value is switched between two states on each invocation of the function.

Example:

Copy
337function flipFullStop() external {
338    if (msg.sender != firstResponder) revert NotFirstResponder();
339    fullStop = !fullStop;
340}

Recommendation:

This type of function is highly discouraged in a blockchain deployment as transaction ordering, pending transactions and other similar blockchain-specific caveats can cause repeat invocations of the same function to occur thereby leading to an undesirable final state. We advise all functions to be adjusted to setter functions instead preventing this form of ambiguity from ever arising.

Alleviation:

The toggle function has been properly adjusted to a setter function thereby alleviating this exhibit.

VPF-03M: Improper Pausability Role Control

Type	Severity	Location
Logical Fault		VendorPoolFactory.sol:L287-L295

Description:

The pause and unpause functions permit only the firstResponder to activate either status and do not allow the owner of the contract to do so.

Impact:

The system currently suffers from a Single-Point-of-Failure (SPoF) as only one role is capable of pausing the system and replacing the role would take additional steps that could be crucial in an emergency situation.

Example:

Copy
287function pause() public {
288    if (msg.sender != firstResponder) revert NotFirstResponder();
289    _pause();
290}
291
292function unpause() public {
293    if (msg.sender != firstResponder) revert NotFirstResponder();
294    _unpause();
295}

Recommendation:

It is highly advisable to permit the owner to control the paused and unpaused state of the contract as well in case either party is not available during a crisis.

Alleviation:

The code was updated to introduce the onlyOwnerORFirstResponder function invoked in place of the first responder checks thus allowing both the owner and the first responder to execute the referenced functions.

VPF-04M: Potentially Improper Discount Deduction

Type	Severity	Location
Logical Fault		VendorPoolFactory.sol:L315-L319

Description:

The maximum discount deduction methodology entirely relies on the discount of the lending asset and does not factor in the collateral asset discount, instead overwriting any previous value with the latest one.

Impact:

The current behaviour is not definitive and can cause a lower collateral discount than expected due to the NFT identification algorithm favouring the higher lending asset discount.

Example:

Copy
315if (block.timestamp < expiry && curCount + 1 <= maxCount && discount >= maxDiscount){
316    maxDiscount = discount;
317    licId = id;
318    maxColDiscount = colDiscount;
319}

Recommendation:

We advise the system to become more deterministic by either deducing the maximum discount on either asset (ideal for users) or enforcing that a maximum discount of the lending asset guarantees a higher than previously recorded discount of the collateral asset (current behaviour enforced by additional checks at the NFT level). While the current behaviour can remain, it relies on proper off-chain definition of the NFT discount items which is ill-advised.

Alleviation:

The code has been updated to rely on a deterministic evaluation of a single license ID per pool thereby ensuring that the discounts offered are tied to a single license, eliminating the need for an expensive loop and ensuring that discounts are known ahead of time during a transaction's submission to the network. As a result, we consider this exhibit adequately addressed.

VPF-05M: Potentially Incorrect State Enforcement

Type	Severity	Location
Logical Fault		VendorPoolFactory.sol:L218, L228, L235, L241, L249, L258, L267, L276

Description:

The linked whenNotPaused modifiers are applied on functions that adjust sensitive contract variables. As such, it is advisable to instead enforce a whenPaused modifier to prevent sensitive contract changes while the contract is in operation.

Impact:

Improper state transitions can cause pending transactions to misbehave when they are ultimately executed as the contract's configuration can currently change between a transaction's submission and a transaction's execution in the network.

Example:

Copy
216function setImplementation(address _implementation)
217    external
218    whenNotPaused
219{
220    onlyOwner();
221    rollBackImplementation = poolImplementationAddress;
222    poolImplementationAddress = _implementation;
223}

Recommendation:

We advise the linked modifiers to be replaced by their paused counterparts to ensure expectable contract operation.

Alleviation:

The Vendor Finance team evaluated this exhibit and opted not to apply a remediation for it as they deem the current functionality of the codebase desirable as the end-user impact will be minimal based on their assessment. As a result, we consider this exhibit acknowledged.