search

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

2 stars 1 forks source link

Incorrect hardcoded USDB and WETH asset address for Blast mainnet, leading to USDB and WETH rewards not claimable on Blast mainnet. #13

Open c4-bot-10 opened 8 months ago

c4-bot-10 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-cfmm/contracts/ThrusterYield.sol#L10-L11 https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-clmm/contracts/ThrusterPool.sol#L43-L44

Vulnerability details

Impact

Currently, Blast testnet addresses for USDB and WETH are hardcoded in contracts. Because the USDB and WETHB addresses for Blast mainnet will be different from testnet (based on Blast doc), this will surely DOS USDB and WETH rewards claiming on Blast mainnet.

USDB and WETH rewards will be locked forever.

Proof of Concept

USDB and WETH are part of Blast native rewards. These addresses are currently hardcoded in ThrusterYield.sol and ThrusterPool.sol.

However, the hardcoded addresses are only for Blast Testnet and will be different for Blast Mainnet. According to Blast doc, these addresses will be slightly different on the Blast mainnet and one should Be careful hardcoding the testnet USDB address for contracts you intend to deploy to mainnet.

This means ThrusterYield.sol and ThrusterPool.sol will only be able to claim yield on Testnet, but will never be able to claim yield on Blast Mainnet where real yield is distributed.

(1) CFMM:

//thruster-protocol/thruster-cfmm/contracts/ThrusterYield.sol
    IERC20Rebasing public constant USDB =
        IERC20Rebasing(0x4200000000000000000000000000000000000022);
    IERC20Rebasing public constant WETHB =
        IERC20Rebasing(0x4200000000000000000000000000000000000023);
...
    function claimYieldAll(
        address _recipient,
        uint256 _amountETH,
        uint256 _amountWETH,
        uint256 _amountUSDB
    )
...
        //@audit claim() will always revert on Blast mainnet due to hardcoded WETHB and USDB address are incorrect.
|>      amountWETH = IERC20Rebasing(WETHB).claim(_recipient, _amountWETH); //@audit-info note: claim WETH yield
|>      amountUSDB = IERC20Rebasing(USDB).claim(_recipient, _amountUSDB);
...

(https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-cfmm/contracts/ThrusterYield.sol#L10-L11)

(2)CLMM:

//thruster-protocol/thruster-clmm/contracts/ThrusterPool.sol
    IERC20Rebasing public constant USDB =
        IERC20Rebasing(0x4200000000000000000000000000000000000022);
    IERC20Rebasing public constant WETHB =
        IERC20Rebasing(0x4200000000000000000000000000000000000023);
...
    function claimYieldAll(
        address _recipient,
        uint256 _amountETH,
        uint256 _amountWETH,
        uint256 _amountUSDB
    )
...
        //@audit claim() will always revert on Blast mainnet due to hardcoded WETHB and USDB address are incorrect.
|>      amountWETH = IERC20Rebasing(WETHB).claim(_recipient, _amountWETH);
|>      amountUSDB = IERC20Rebasing(USDB).claim(_recipient, _amountUSDB);
       //@audit Gas rewards will also be locked forever due to this is the only function to claim gas.
|>      amountGas = IBlast(BLAST).claimMaxGas(address(this), _recipient);

(https://github.com/code-423n4/2024-02-thruster/blob/3896779349f90a44b46f2646094cb34fffd7f66e/thruster-protocol/thruster-clmm/contracts/ThrusterPool.sol#L811-L812)

In addition, In ThrusterPool.sol(CLMM), since there's no separate function to claim gas rewards, this also means gas rewards will also be locked forever due to claimYieldAll reverts.

Tools Used

Manual

Recommended Mitigation Steps

Do not hardcode USDB and WETHB addresses; set them through the constructor.

Assessed type

Error

jooleseth commented 8 months ago

This is not an issue. We need to hardcode them instead of setting them in the constructor due to contract size limitations for ThrusterPool. We are cognizant that the addresses need to be switched over to the mainnet prior to deployment

flowercrimson commented 8 months ago

(1) I think the code should be evaluated against Blast L2 mainnet deployment. Any contract code in scope that only works for testing purposes but will break in production runtime should be considered a bug. In this case, this will cause loss of rewards and DOS claiming. We also cannot infer from the audit scope what the protocol plans to modify later. (2) ThrusterYield.sol shouldn't have a contract size limitation issue and can be refactored. For ThrusterPool.sol, it might help to change the optimizer setting.

c4-judge commented 8 months ago

0xleastwood marked the issue as satisfactory

0xEVom commented 8 months ago

I agree with the sponsor here. This is the same as #3 and should also be invalid imo:

flowercrimson commented 8 months ago

@0xEVom

At the time of the audit, the Blast official docs (which have since been updated) listed these as the L2 addresses without any mention of testnet.

This is incorrect. Blast has clearly stated that the hardcoded address is NOT intended for the mainnet and the address will be different on mainnet in multiple locations (including the link embedded in your comment). See screen shots below on Blast doc.

Screenshot 2024-02-28 at 9 26 58 AM

image image

Per the contest docs "Any deployment errors are out of scope, e.g. incorrect constructor arguments". I would say this clearly counts as such.

This is not a deployment error. The addresses are hardcoded in the contract and they are not configurable constructor arguments.

The fact that the addresses are hardcoded is not an issue in itself, only that the outdated/testnet values are used.

Hardcoded incorrect values that will cause rewards DOS and loss of rewards are medium severity impacts.

0xEVom commented 8 months ago

It looks like you just missed my edit. See above—these contracts are not deployable in their current state.

jooleseth commented 8 months ago

I agree with @0xEVom, the contracts would revert if the hardcoded addresses are wrong. This is because we call functions in the constructor to configure the contracts for Blast. Don't believe this should be a Medium severity, maybe Low, but we need to hardcode addresses for the V3 Pool due to contract size, so to stay consistent, we hard coded it on all surfaces

c4-sponsor commented 8 months ago

jooleseth (sponsor) disputed

c4-judge commented 8 months ago

0xleastwood changed the severity to QA (Quality Assurance)