Griefing ERC3156FlashBorrower by calling flashloan on his behalf

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L609-L631

Vulnerability details

Impact

Any user can drain ERC3156FlashBorrower contract ibt balance, if it doesn’t check who is initiator. That way any user can call PT.flashLoan() and the fee in form of ibt will be transferred to the PT contract. The user will not be able to redeem this, since the fees are only for the owners, but he still be able to lower ERC3156FlashBorrower balance.

Proof of Concept

The user needs to find an ERC3156FlashBorrower that has an internal balance of ibt on and call PT.flashLoan(), this way the fee will be transferred with the flash loan amount at the end. The user will only pay the fee, but since the protocol will be used on Polygon, Arbitrum, it will not be a bad incentive for the user to drain it little by little.

function flashLoan(
    IERC3156FlashBorrower _receiver,
    address _token,
    uint256 _amount,
    bytes calldata _data
) external override returns (bool) {
    if (_amount > maxFlashLoan(_token)) revert FlashLoanExceedsMaxAmount();

    uint256 fee = flashFee(_token, _amount);
    _updateFees(fee);

    // Initiate the flash loan by lending the requested IBT amount
    IERC20(ibt).safeTransfer(address(_receiver), _amount);

    // Execute the flash loan
    if (_receiver.onFlashLoan(msg.sender, _token, _amount, fee, _data) != ON_FLASH_LOAN)
        revert FlashLoanCallbackFailed();

    // Repay the debt + fee
    IERC20(ibt).safeTransferFrom(address(_receiver), address(this), _amount + fee);

    return true;
}

Tools Used

Manual Review

Recommended Mitigation Steps

It's hard to mitigate this, but one thing could be to only allow the ERC3156FlashBorrower _receiver to call this function, as shown in EIP3156, where it checks if the initiator is the FlashBorrower contract.

Assessed type

DoS

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #46

[gzeon-c4]

note approval need to be set, receiver implementation error

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid

[Slavchew]

Hi @JustDravee,

I think this one was wrongly flagged as unsatisfactory.

Based on the sponsor's response in #46

That being said, the above issue contains a small part of correct information, that is better explained here: #113.

Hence we agree there is an issue but strongly disagree with the severity. We consider the real issue as a medium as it is a grief and the fees are possessed by a trusted admin.

He agrees that it is a medium severity.

There are also other valid duplicates of this: #32, #46 (50%)

Please review this again, Thanks!

[kazantseff]

Hey @Slavchew, The msg.sender calling flashLoan() is getting passed to onFlashLoan() callback as an initiator, so wouldn't it be the flashBorrower mistake to not check if the initiatior is an allowed address? This is also how it's done in the EIP-3156 Flash Borrower reference implementation, so I don't see how is this an error on a Spectra end and not on the end of the flashBorrower?

[Slavchew]

Hey @Slavchew, The msg.sender calling flashLoan() is getting passed to onFlashLoan() callback as an initiator, so wouldn't it be the flashBorrower mistake to not check if the initiatior is an allowed address? This is also how it's done in the EIP-3156 Flash Borrower reference implementation, so I don't see how is this an error on a Spectra end and not on the end of the flashBorrower?

Hey @kazantseff Yes, the problem is when the flashBorrower doesn't check if the initiator is trusted. But it can still check if msg.sender(FlashLender) is the Spectra PT contract, making the situation exploitable. EIP-3156 notes in the Security Considerations section that it's good to include initiator checks, but it's not part of the spec that developers follow when implementing something like this. A common issue is developers missing certain EIP checks or overlooking security considerations. Even a real example can be seen here in the Spectra codebase, where they missed some EIP-5095 parts (#210), as with many other protocols.

Considering this, I believe that a misimplemented FlashBorrower could be vulnerable even if ensuring that msg.sender (FlashLender) is a Spectra PT, and with our recommendation, even if FlashBorrower is implemented incorrectly, the problem is mitigated.

[kazantseff]

@Slavchew, The point is that contracts in question for this audit is contracts built by Spectra, and as far as I can tell, Spectra's PT contract does indeed follow the ERC-3156 spec, when it comes to implementing a flash loan contract. If a flashBorrower built on top of PT contract is vulnerable, it's not a problem introduced by a Spectra team, thus it's not something they should fix. I think that we should assume that integrating contract does follow the spec, because security considerations section is one of the most important parts in the spec and it's not something easy to miss. I don't think it's reasonable to expect Spectra to adjust to every possible way a flashBorrower can be wrongly implemented, since otherwise we could come up with many more findings based on many more integrating contracts that do not follow the spec they claim to follow. I think it falls down into the category of user mistakes, since it would be a mistake of a flashBorrower to not check the flashLoan function and to not implement his contract correctly.

Also your mitigation makes PT contract differ from the specification, which in my opinion does not make sense, and may make other flashBorrower contracts that follow the spec become incompatible.

[Slavchew]

@Slavchew, The point is that contracts in question for this audit is contracts built by Spectra, and as far as I can tell, Spectra's PT contract does indeed follow the ERC-3156 spec, when it comes to implementing a flash loan contract. If a flashBorrower built on top of PT contract is vulnerable, it's not a problem introduced by a Spectra team, thus it's not something they should fix. I think that we should assume that integrating contract does follow the spec, because security considerations section is one of the most important parts in the spec and it's not something easy to miss. I don't think it's reasonable to expect Spectra to adjust to every possible way a flashBorrower can be wrongly implemented, since otherwise we could come up with many more findings based on many more integrating contracts that do not follow the spec they claim to follow. I think it falls down into the category of user mistakes, since it would be a mistake of a flashBorrower to not check the flashLoan function and to not implement his contract correctly.

Also your mitigation makes PT contract differ from the specification, which in my opinion does not make sense, and may make other flashBorrower contracts that follow the spec become incompatible.

Fair point on most things. I can only argue about the ERC-3156 compliance on the PT contract based on this response from the sponsor:

I think there is no need for more spam on this matter, the judge and the sponsor will decide.

[JustDravee]

Hey there, the answer would follow what I explained in the duplicate: https://github.com/code-423n4/2024-02-spectra-findings/issues/32#issuecomment-1996586164 I don't consider this as reasonably being the protocol's responsibility, so even if the Spectra Team could've acknowledged a Medium Severity finding on an audit, this isn't one I can accept on code4rena.