Spectra - Shubham's results

gzeon-c4 marked the issue as primary issue

gzeon-c4 marked the issue as duplicate of #33

gzeon-c4 marked the issue as sufficient quality report

JustDravee marked the issue as partial-25

JustDravee marked the issue as satisfactory

JustDravee marked the issue as partial-25

@JustDravee Wondering why this is partial-25 whereas issue #116 is partial-75?

@Shubh0412 because that person had several submissions under the duplicated finding and I totaled them

gzeon-c4 marked the issue as insufficient quality report

gzeon-c4 marked the issue as duplicate of #46

JustDravee marked the issue as unsatisfactory: Invalid

This issue is not a duplicate of #46

The mentioned issue has been reported in the past in another contest- Link The attack path, the erc implemented (ERC3156FlashBorrower) & the impact due to the vulnerability is the same.

There are also other consequences if the vulnerability described above which is not checking the balance of the contract before & after a flashloan.

The other scenario can be if the underlying token (widely used) which is compliant but is later upgraded & introduces vulnerability (non intentional) & steals funds from the contract during the flashloan callback like described in this report - Link The likelihood of that happening is quite low, although it may lead to stealing of funds from the contract. But mostly likely it would have been labelled under the known issues mentioned in the docs "The Principal Token contract expects a compliant ERC4626 and non malicious IBT". Therefore did not report that.

However, the likelihood of the situation happening in the submitted finding is high. As mentioned in the impact section, the user ends up paying double the amount.

Fixing the vulnerability would solve two issues:

• User's funds being stolen
• Future upgradation of an existing compliant IBT to a vulnerable one which leads to freezing or a griefing attack.

Therefore this issue comes under Medium severity risk because at present, the user's assets are at risk.

Hello @Shubh0412

This is interesting, thank you for the thorough and high quality explanation.

Yes, this isn't a duplicate of 46, indeed.

However I disagree with https://github.com/sherlock-audit/2023-01-ajna-judging/issues/33 being accepted as a Medium. Yes, it happened on Sherlock, but I disagree with that decision. There'd need to be several self-rekt factors in, like the flashloan receiver giving max approval and actually already holding the funds it's trying to flashloan. But more importantly, it'd need to deviate from the recommendations. See on https://eips.ethereum.org/EIPS/eip-3156#flash-borrower-reference-implementation how flash loan are supposed to be initiated:

 /// @dev ERC-3156 Flash loan callback
    function onFlashLoan(
...
    ) external override returns(bytes32) {
...
        return keccak256("ERC3156FlashBorrower.onFlashLoan");
    }
    /// @dev Initiate a flash loan
    function flashBorrow(
        address token,
        uint256 amount
    ) public {
...
        IERC20(token).approve(address(lender), _allowance + _repayment);
        lender.flashLoan(this, token, amount, data);
    }

Also, the security recommendations say this:

 Flash lending security considerations
 Automatic approvals
The safest approach is to implement an approval for amount+fee before the flashLoan is executed.

And the Rationale says this:

 Rationale
The amount + fee are pulled from the receiver to allow the lender to implement other features that depend on using transferFrom, without having to lock them for the duration of a flash loan. An alternative implementation where the repayment is transferred to the lender is also possible, but would need all other features in the lender to be also based in using transfer instead of transferFrom. Given the lower complexity and prevalence of a “pull” architecture over a “push” architecture, “pull” was chosen.

Therefore, the exploit here would be relying on a mis-implementation for the flashloan borrower, which isn't in the scope of what Spectra is supposed to be protecting against (there is a standard => follow it).

This is at most a self-rekt from the receiver, and wouldn't hurt Spectra in any way.

Also, the 2nd bug https://github.com/sherlock-audit/2023-01-ajna-judging/issues/101 aims to work with any kind of arbitrary fungible tokens. This isn't the case here, but you indeed already mention it. It's indeed here out of scope with IBT.

I agree however that your recommendation of checking the balance of the contract before & after a flashloan would be an extra layer of security for the protocol. So I'll downgrade to QA instead of invalidating.

JustDravee marked the issue as not a duplicate

JustDravee changed the severity to QA (Quality Assurance)

JustDravee marked the issue as grade-b

Thank you @JustDravee for the details.

However in the above scenario, if the user does not have an amount equivalent to the flash loan amount, the function would simply revert & the user might attempt a second time & it all goes well. But still if the required conditions are met & the user does no mistake on their part, they are robbed of their existing funds. Even if this doesn't hurt spectra pool, the user would still blame the protocol for stealing.

However "Not checking the balance" would still be an issue apart from just being an additional layer of security.

Although certain edge cases must be met in order to execute the attack, the issue still comes under Medium Severity.

The SafeERC20 library only performs safe checks IF the token returns a bool, which is a common misconception among engineers and isn't foolproof. Take the case of USDT for example as stated in the natspecs of the SafeERC20 contract, which the PrincipalToken contract inherits.

File: SafeERC20.sol
     /**
     * @dev Set the calling contract's allowance toward `spender` to `value`. If `token` returns no value,
     * non-reverting calls are assumed to be successful. Meant to be used with tokens that require the approval
     * to be set to zero before setting it to a non-zero value, such as USDT.
     */

Actual Code - Link

File: SafeERC20.sol

79       if (!_callOptionalReturnBool(token, approvalCall)) {
80            _callOptionalReturn(token, abi.encodeCall(token.approve, (spender, 0)));
       ...

Because the flashloan does not assert that the balance of the pool is consistent before and after the flashloan, an attacker can steal the sum of tokens which had been flashloaned. (Now this affects Spectra protocol)

Absence of just a simple check leads to vulnerability both impacting the protocol as well as the user. Given the impact, the issue should be considered of a higher severity than a low one.

@Shubh0412 , While I really like your style of explaining and the details you give (really), mentioning USDT in your arguments, which isn't an ERC20-Compliant token, isn't in your favor. Here we're talking about IBT, which is an ERC4626 (inheriting ERC20) Compliant token. I cannot accept weird tokens as more than QAs (even doing a favor here by not putting this as OOS), since the protocol explicitly said on the contest README:

The Principal Token contract expects a compliant ERC4626 and non malicious IBT

The protocol relies on trusting the integrated IBT. A malicious actor could create a malicious compliant (EIP4626) IBT that could break the protocol and might result in a loss of funds for the users.

So, the compliance is expected (even non-malicious IBTs must be compliant). Non-compliance edge-cases are out-of-scope. I might've accepted it if it wasn't explicitly mentioned in this contest's rules. But it was.

@JustDravee Really appreciate the fact that you have been so patient & understanding throughout the post judging period, specially with me.

I wish I had another powerful comeback, but only to face defeat like the other two times. But third time's a charm.

My only counter statement/point of view against the README would be of the use of the word "create".

A malicious actor could create a malicious compliant (EIP4626) IBT

The above argument would have been OOS if, as stated in the README, a malicious actor created a malicious IBT. Then it would have been the user's fault interacting with it & would have come under known issues.

But the fact that already existing popularly used tokens are integrated in the protocol, gives opportunity to certain edge cases as stated along the length of the above discussions.

Had it only been an edge case regarding weird tokens, there would have been no point where I kept nitpicking about the same fault over & over until this whole discussion turned ugly as have happened a few times in the past during the post judging period by others.

However the underlying vulnerability has 3 different impacts. 2 affecting the protocol & 1 affecting the user.

1. Future upgradation of underlying token introducing a bug that an attacker exploits before it is known to common public
2. Existing weird popular tokens that return true for non reverting calls which would ultimately steal the whole flashloaned amount
3. A user losing double the flashloaned amount

Summing up all the above factors, taking the loophole in the README & the 3 impacts of the underlying vulnerability into consideration, I plead the above issue to be deemed Medium. I rest my case.

@Shubh0412, if we play on words, the title said "The Principal Token contract expects a compliant ERC4626 and non malicious IBT" Let's remove the malicious part: "The Principal Token contract expects a compliant ERC4626 IBT" I praise your effort in trying to convince, and will give you that the remediation has some potential value, but I cannot upgrade this to a Medium Severity finding. However, I can give this report (which should include our discussion on the issue's value) a grade-A as this could serve as a thorough analysis of an acknowledged admin-related (on the receiver) risk issue.

JustDravee marked the issue as grade-a