VerifyPaymentPortal Manual Review Findings

VPP-01M: Unconsumed Token Limits

Type	Severity	Location
Logical Fault		VerifyPaymentPortal.sol:L97

Description:

The token limit enforced by the contract is not consumed thereby permitting multiple afterApprove invocations to successfully execute if they all fall below the token limit even though they cumulatively may surpass it.

Impact:

This exhibit in combination with inexistent access control can lead to harmful siphoning of user funds. Overall, the approval system should operate under a first-come-first-serve basis and correctly consume the token amount denoted by its approver.

Example:

Copy
88function afterApprove(address approver_, Evidence[] calldata evidences_)
89    external
90{
91    PaymentConfig memory paymentConfig_ = abi.decode(
92        SSTORE2.read(paymentConfigPointer),
93        (PaymentConfig)
94    );
95    uint256 amount_ = calculateUSDTtoTKN(paymentConfig_.usdtPrice);
96    require(
97        amount_ <= tknLimits[approver_],
98        "VerifyPaymentPortal: TKN_LIMIT"
99    );
100    for (uint256 i_ = 0; i_ < evidences_.length; i_++) {
101        // Override first share recipient as approved account.
102        paymentConfig_.shares[0].recipient = evidences_[i_].account;
103        SplitPayment.splitTransfer(
104            paymentConfig_.tkn,
105            approver_,
106            amount_,
107            paymentConfig_.shares
108        );
109    }
110}

Recommendation:

We advise the limit to be properly consumed and updated in storage disallowing this scenario from ever unfolding.

Alleviation:

The Alliance Block team stated that the token limit is meant to function as a per-approval sensible "price" limit of the USDT denominated in TKN. As a result, we consider this exhibit nullified given that the initial implementation achieves the purpose that Alliance Block stated. As a final note, we advise the documentation to be expanded to explicitly state that the tknLimits is acting as a per-approval limit to avoid such ambiguities in the future.

VPP-02M: Inexistent Access Control of Approve Hook

Type	Severity	Location
Logical Fault		VerifyPaymentPortal.sol:L88-L90

Description:

The afterApprove hook does not properly impose access control on its callers permitting arbitrary parties to specify an approver_ who has provided allowance to the contract and fake evidences to acquire the first share percentage of the share configuration for themselves.

Impact:

It is currently possible to sabotage allowances of other users for valid approvals by spoofing the evidences provided to the contract. This type of attack is especially exploitable under race conditions whereby a pending evidence submission is identified and re-submitted with a different beneficiary and a higher gas fee.

Example:

Copy
81/// We require that the approver pays each of the approvees as gratuity for
82/// access to the private data required for completion of the KYC process.
83/// Every payment is the same for every approval at the current fx rates.
84/// Every payment is per-approval and subject to the cost limit set by the
85/// approver. The approver MUST set a limit for themselves as the unset
86/// costLimit is zero.
87/// @inheritdoc IVerifyCallback
88function afterApprove(address approver_, Evidence[] calldata evidences_)
89    external
90{
91    PaymentConfig memory paymentConfig_ = abi.decode(
92        SSTORE2.read(paymentConfigPointer),
93        (PaymentConfig)
94    );
95    uint256 amount_ = calculateUSDTtoTKN(paymentConfig_.usdtPrice);
96    require(
97        amount_ <= tknLimits[approver_],
98        "VerifyPaymentPortal: TKN_LIMIT"
99    );
100    for (uint256 i_ = 0; i_ < evidences_.length; i_++) {
101        // Override first share recipient as approved account.
102        paymentConfig_.shares[0].recipient = evidences_[i_].account;
103        SplitPayment.splitTransfer(
104            paymentConfig_.tkn,
105            approver_,
106            amount_,
107            paymentConfig_.shares
108        );
109    }
110}

Recommendation:

We advise proper access control to be applied by the payment portal ensuring that the caller is a whitelisted Alliance Block system module that is meant to integrate with the contract.

Alleviation:

The Ownable contract has been introduced to the VerifyPaymentPortal contract's inheritance chain and the afterApprove function is now properly guarded with an onlyOwner modifier, preventing misuse by arbitrary parties. As a final alleviation, the documentation of the contract was also expanded to state who the intended owner is and what steps the deployer should take to ensure the system works properly with the Verify contract of the Alliance Block ecosystem.