Description

The calculateAndBurnProofFee and createProofSet functions automatically refund the excess funds to the msg.sender account using the transfer function.

function calculateAndBurnProofFee(uint256 setId, uint256 gasUsed) internal {
    [...]
    if (msg.value > proofFee) {
        // Return the overpayment
        payable(msg.sender).transfer(msg.value - proofFee);
    }
    emit ProofFeePaid(setId, proofFee, filUsdPrice, filUsdPriceExpo);
}

Since the transfer function is used, only a limited amount of gas is sent with the transfer, and if the call to the sender address reverts, then the current call context will also revert.

Impact

Although the gas limit prevents most reentrancy attacks, using transfer is generally discouraged↗ due to gas prices potentially changing in future Ethereum forks.

Additionally, if the sender is a smart contract such as a smart wallet, a governance contract, or a DAO, then it may have complex logic in its receive handler that costs more gas to execute, or it may not even have a payable receive handler. In both cases, this transfer will revert, which makes calling provePossession properly difficult for these users. They will either need to work with a block builder to accurately predict the price exactly when their transaction executes or implement custom logic that calculates the price exactly outside the call to provePossession.

Recommendations

Instead of automatically returning funds, consider implementing the common pattern of storing a balance per user and allowing users to withdraw their balance at any time upon request. This approach improves compatibility because the funds will remain in the contract and, if the caller contract is willing to pay the gas for the withdrawal or upgrades itself in the future to allow for payable transfers, then they can receive the funds they are owed. It can be implemented by tracking overpayments in a mapping, adding to the value whenever an overpayment would otherwise be sent, and then implementing a dedicated withdrawal function.

Separately, for the transfer itself, in order to send the rest of the gas instead of only a limited amount of gas, consider using the msg.sender.call.value(value)("") syntax instead of transfer:

(bool success, ) = msg.sender.call.value(msg.value - proofFee)("");
require(success, "Transfer failed.");

If more gas is supplied with the call, ensure that reentrancy cannot become an issue, especially if the second recommendation is implemented without the first. See Finding ref↗ for more information.

Remediation

This issue has been acknowledged by Filecoin, and a fix was implemented in commit d6b0a639↗.

Filecoin provided the following response:

Moving transfer call to the end of the provePossession call and createProofSet call.