code-423n4 / 2024-02-thruster-findings

2 stars 1 forks source link

Incorrect gas claiming logic in ThrusterPoolDeployer #24

Open c4-bot-6 opened 7 months ago

c4-bot-6 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-clmm/contracts/ThrusterPoolDeployer.sol#L45-L47

Vulnerability details

Impact

From the contest documentation:

All contracts that use gas should comply with the Blast gas claim logic.

However, the ThrusterPoolDeployer contains a flaw in the implementation of claimGas() which will prevent it from ever claiming the gas it induces. The function attempts to claim gas for the zero address (address(0)) instead of the deployer's own address (address(this)):

    function claimGas(address _recipient) external override onlyFactory returns (uint256 amount) {
        amount = IBlast(BLAST).claimMaxGas(address(0), _recipient);
    }

This misconfiguration prevents the ThrusterPoolDeployer from reclaiming any gas, as the IBlast.claimMaxGas() call will always fail when provided with the zero address.

Proof of Concept

https://github.com/blast-io/blast/blob/master/blast-optimism/packages/contracts-bedrock/src/L2/Blast.sol#L274-L283

/**
 * @notice Claims gas available to be claimed at max claim rate for a specific contract. Called by an authorized user
 * @param contractAddress The address of the contract for which maximum gas is to be claimed
 * @param recipientOfGas The address of the recipient of the gas
 * @return The amount of gas that was claimed
 */
function claimMaxGas(address contractAddress, address recipientOfGas) external returns (uint256) {
    require(isAuthorized(contractAddress), "Not allowed to claim max gas");
    return IGas(GAS_CONTRACT).claimMax(contractAddress, recipientOfGas);
}

Tools Used

Manual review

Recommended Mitigation Steps

Use address(this) rather than 0.

Assessed type

Other

jooleseth commented 6 months ago

I agree, we caught this issue a few days ago too. Blast had initially in their docs specified it should be address(0) instead of address(this) for gas claiming and we missed changing this in the audit commit freeze. Will agree to a Medium Risk bug for this, as it doesn't affect any user funds

c4-judge commented 6 months ago

0xleastwood marked the issue as primary issue

c4-judge commented 6 months ago

0xleastwood marked the issue as satisfactory

c4-sponsor commented 6 months ago

jooleseth (sponsor) acknowledged

c4-sponsor commented 6 months ago

jooleseth (sponsor) confirmed

c4-judge commented 6 months ago

0xleastwood marked the issue as selected for report