Rhaydden - Potential ERC20 Token Approval Issue in PeripheryPayments Contract

[sherlock-admin2]

Rhaydden

medium

Potential ERC20 Token Approval Issue in PeripheryPayments Contract

Summary

The PeripheryPayments.sol uses safeTransferFrom for token transfers, but it does not explicitly enforce the requirement for the payer to have set an appropriate allowance for the contract to transfer tokens on their behalf. This could lead to transactions failing if the allowance is not set correctly.

Vulnerability Detail

The contract assumes that the payer has already approved the contract to transfer the necessary amount of tokens. If the payer has not set an appropriate allowance, the safeTransferFrom function will fail. This failure is due to how the Ethereum ERC20 token standard operates, specifically with the safeTransferFrom function used in the contract. This is not an arbitrary issue but a requirement for the function to work correctly, as per the ERC20 token standard. The contract does not include a mechanism to check or enforce the allowance before attempting the transfer.

Impact

If the payer has not set an appropriate allowance for the contract, the _pay function will fail when attempting to transfer tokens on behalf of the payer. This could lead to failed transactions and a poor user experience, potentially affecting the functionality of any dApps or services that rely on this contract for token transfers.

In order to prove my point why the _pay function will fail, I will give some scenarios:

As seen in the contract, the _pay function includes the following logic for handling token transfers:

_1. If the token being paid with is WETH9 and the contract has sufficient Ether to cover the payment, it wraps the necessary Ether and transfers it.

2. If the payer is the contract itself, it uses safeTransfer to transfer tokens directly.
3. If neither of the above conditions is met, it assumes the contract needs to pull tokens from the payer using safeTransferFrom._

In the third scenario, where the contract needs to pull tokens from the payer using safeTransferFrom, it relies on the assumption that the payer has set the appropriate allowance for the contract to transfer tokens on their behalf.

If the payer has not set the correct allowance for the contract, the safeTransferFrom operation will revert, and the _pay function will fail. This is because the ERC-20 standard requires that the allowance is set before a contract can transfer tokens on behalf of the token owner.

Therefore, in the scenario where the payer has not set an appropriate allowance for the contract, the _pay function will indeed fail when attempting to transfer tokens on behalf of the payer using safeTransferFrom.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/base/PeripheryPayments.sol#L82-L104

Tool used

Manual Review

Recommendation

Consider implementing a check within the contract to ensure that the payer has set an appropriate allowance for the contract before attempting the transfer. This can be done by calling the allowance function of the ERC20 token contract and verifying that the returned value is sufficient for the transfer. Example:

function _pay(address token, address payer, address recipient, uint256 value) internal {
    // Check if the payer has enough allowance for the contract
    uint256 allowance = IERC20(token).allowance(payer, address(this));
    require(allowance >= value, "Insufficient allowance for transfer");`

    if (token == address(WETH9) && address(this).balance >= value) {
        // Pay with WETH9
        WETH9.deposit{value: value}(); // Wrap only what is needed to pay
        WETH9.transfer(recipient, value);