search

code-423n4 / 2022-02-tribe-turbo-findings

1 stars 0 forks source link

QA report #41

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboSafe.sol#L311-312

Vulnerability details

Impact

Vault shares and collateral cTokens might be accessed with sweep if a cToken have more than one address.

This will disturb the system accounting. The tokens caller obtained will be at a loss for other safe users who will not be able to receive Fei they own per contract records.

This is a fund loss scenario, but the probability is low as such tokens are rare, so setting severity to medium

Proof of Concept

TurboSafe.sweep controls for one vault and one cTokens address, allowing for sweeping everything else:

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboSafe.sol#L311-312

Some ERC20 have more than one address they can be operated with:

https://github.com/d-xo/weird-erc20#multiple-token-addresses

Recommended Mitigation Steps

As address control is used consider providing the ability to input a list of asset addresses to prohibit operations with

Joeysantoro commented 2 years ago

This is an extremely niche edge case. I'm inclined to think multiple address tokens will not be approved for Turbo. Will contest to low and leave as acknowledged unless @transmissions11 has other ideas

transmissions11 commented 2 years ago

Collateral cTokens having multiple addresses is out of scope because we control the Turbo pool and would never make such a token.

As for Vault shares in theory it could be an issue but once again we can vet Vaults to ensure they're not unsafe like this.

GalloDaSballo commented 2 years ago

I believe the finding to have merit and appreciate the warden's submission.

However I have to agree with the sponsor in that there is no reason to deploy a second cToken with the same underlying and if that were to happen remediation would be very quick.

So I'll mark the finding as valid but non-critical as it technically is correct, could happen but ultimately is not a vulnerability

GalloDaSballo commented 2 years ago

Will judge as a separate report 3/10

GalloDaSballo commented 2 years ago

Crazy how with the TUSD Compound Disclosure this finding takes a different meaning.

With the additional information I have : https://medium.com/chainsecurity/trueusd-compound-vulnerability-bc5b696d29e2

I believe the finding is of Low Severity and will re-rate to 5/10 as a single QA report.

Ultimately I believe this can happen and if that were the case the finding could have a high impact, however the likelihood is extremely low, and would require the sponsor to mistakenly add a token with multiple address.

CloudEllie commented 2 years ago

Since this issue was downgraded to a QA level, and the warden did not submit a separate QA report, we've renamed this one to "QA report" for consistency.

The original title, for the record, was "TurboSafe.sweep can't control several addresses per token."