search

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

1 stars 0 forks source link

QA report #35

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/TurboSafe.sol#L335 https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/TurboSafe.sol#L113

Vulnerability details

Impact

The safe owner can call gib function in TurboSafe.

In other words, the creator of a safe can impound (or steal) any amount of the underlying token available (and transfer it to any address).

The reason is that the requiresLocalOrMasterAuth modifier in the TurboSafe contract skips all auth checks if the msg.sender is the owner.

Proof of Concept

  1. Create a safe via the TurboRouter's createSafe function.
  2. Let users have time to deposit (the owner can even set a new authority that allows anybody to deposit)
  3. The owner calls gib and transfers all the underlying to themselves.

Tools Used

Manual review.

Recommended Mitigation Steps

One way of mitigating the issue is for the gib function to not rely on the requiresLocalOrMasterAuth modifier but instead have its own modifier-auth-check to check if the msg.sender is allowed to call the gib function.

transmissions11 commented 2 years ago

i agree this is not desired behavior but dispute the severity, whats the issue? they still cant withdraw without repaying their debt

Joeysantoro commented 2 years ago

Disputing to low

GalloDaSballo commented 2 years ago

The warden showed how the owner can break the accounting of safe by using gib, this is contingent on them doing it to their own safe so Medium severity would be more appropriate.

However, impact is further reduced as the function redeemUnderlying by the cToken will still protect against withdrawing an undercollateralized amount.

This, paired with the fact that the owner is basically breaking their own stuff, leads me to agree with a Low Severity

GalloDaSballo commented 2 years ago

Judging as QA Report, 3/10

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 "The safe owner can steal the safe's collateral."