Description

The provePossession function verifies the provided proofs and calculates the amount of gas used to determine the fee amount. If the caller provides more msg.value than required, the excess funds are refunded using transfer function call. However, during this refund process, the caller can attempt to reenter the contract and perform arbitrary actions before the PDPListener(listenerAddr).possessionProven call and proofSetLastProvenEpoch[setId] update.

function provePossession(uint256 setId, Proof[] calldata proofs) public payable{
    [...]
    for (uint64 i = 0; i < proofs.length; i++) {
        [...]
        bytes32 rootHash = Cids.digestFromCid(getRootCid(setId, challenges[i].rootId));
        bool ok = MerkleVerify.verify(proofs[i].proof, rootHash, proofs[i].leaf, challenges[i].offset);
        require(ok, "proof did not verify");
    }
    uint256 gasUsed = (initialGas - gasleft()) + ((calculateCallDataSize(proofs) + 32) * 1300);
    calculateAndBurnProofFee(setId, gasUsed);

    address listenerAddr = proofSetListener[setId];
    if (listenerAddr != address(0)) {
        PDPListener(listenerAddr).possessionProven(setId, proofSetLeafCount[setId], seed, proofs.length);
    }
    proofSetLastProvenEpoch[setId] = block.number;
    emit PossessionProven(setId, challenges);
}

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);
}

Impact

Note that this reentrancy is currently not exploitable due to the transfer function being limited to 2,300 gas, as of the current Ethereum version. However, it is not good practice to rely on this gas limit to prevent reentrancy, because gas costs can change over time. See Finding ref↗ for more detailed information.

Also, while the current SimplePDPService implementation of the PDPListener contract prevents reentrancy exploitation due to block.number validation (if the caller tries to prove possession again and start a new proving period within the same transaction, the PDPListener(listenerAddr).possessionProven call will revert), other PDPListener implementations may not have such verification. This could lead to unintended state changes in the PDPListener contract.

Recommendations

To mitigate this issue, external calls should only be performed after all state changes are finalized. The possible solution is to move the refund operation to the end of the function, after the possessionProven call, ensuring that no unintended reentrant behavior can occur before the PDPListener is notified.

Also, we recommend adding a reentrancy guard to absolutely protect against reentrant calls, both from the sender and from a potential malicious listener.

Remediation

This issue has been acknowledged by Filecoin, and fixes were implemented in the following commits:

• 4b04bc08↗
• 70dbb0e8↗

Filecoin provided the following response:

We’ve decided to still do refund transfer because we control the control proxy and don’t want to control any resident funds. This is a good tradeoff as we don’t expect very sophisticated wallets to act as senders of these messages. We’ve done remediation 2 to maximize compatibility with different senders forwarding the remaining gas limit to the refunding call.