Omniscia Alliance Block Audit
VerifyPaymentPortal Manual Review Findings
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:
88function afterApprove(address approver_, Evidence[] calldata evidences_)89 external90{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_.shares108 );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:
81/// We require that the approver pays each of the approvees as gratuity for82/// 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 the85/// approver. The approver MUST set a limit for themselves as the unset86/// costLimit is zero.87/// @inheritdoc IVerifyCallback88function afterApprove(address approver_, Evidence[] calldata evidences_)89 external90{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_.shares108 );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.