Sturdy contest - mtz's results

Judge has assessed an item in Issue #63 as High risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L129-L134 https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L160-L161

Vulnerability details

Impact

An attacker can use MEV (via gas auction or Flashbots or control of miners) to cause an unfair division of yield. By providing a very large (relative to the size of all other stablecoin deposits combined) stablecoin deposit Just-in-Time before an admin's call to distributeYield the stablecoin deposited by the attacker will receive a very large amount of the yield and the attacker can immediately withdraw their deposit after yield is distributed. We assume this allows an attacker to get a lot of the yield reward even though they haven't provided any deposit that has been borrowed. However, the exact mechanism for how yield is distributed to lenders of a particular stablecoin is in LendingPool.sol, which is out of scope. However it is implied in the documentation of this repo that it is based on the balance of that asset the lender has provided. We have confirmed that in LendingPool.sol the yield is distributed based on the proportion of the asset provided. However, even ignoring this, MEV can still be used to unfairly hurt lenders of other stablecoins.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Proof of Concept

1. An attacker watches the mempool for calls to distrbitueYield by the admin.
2. The attacker orders the block's transactions (most easily using a flashbots bundle) in the following order: i. Attacker deposits stablecoins to lend (ideally the stablecoin will be the one with the least volume). ii. admin's call to distributeYield happens. iii. Attacker withdraws their deposit.

The attacker has thus made the asset they deposited (and thus themselves) receive much of the yield even though they provide no value to Sturdy since none of their deposit is ever borrowed so the never do anything to earn yield for sturdy. This attack can be done by a whale or by borrowing (even from sturdy) assets and converting them to a stablecoins accepted by sturdy before i. and returning them after iii. This will essentially be cost free for the attacker, none of their capital will ever be tied up by borrowers.

Recommended Mitigation Steps

The simplest way to mitigate this is for the admin to use flashbots or some other means of submitting the distributeYield call that skips the mempool. This is only a partial mitigation since attackers can still withdraw right after yield is distributed and get lucky by depositing soon before the distribution thus still capture more yield than they should have. A better mitigation could use something like snapshotting who has deposited since the last yield distribution and only give these depositers yield based on the size of their deposits the next time yield is distributed.

ETH can accidentally be sent to a contract that rejects the transfer because the sent value returned by address(_to).call{value: receivedETHAmount}(''); is not actually checked.

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L140-L142 On line 140 a call is made to transfer ETH (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}(''); Then on line 141 receivedETHAmount is returned. And on line 142 the sending of ETH is checked.

Recommended Mitigation Steps

To fix this, line 141 and line 142 should be swapped.

ETH can accidentally be withdrawn to the 0x0 address.

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/LidoVault.sol#L122-L142

Neither _withdrawFromYieldPool nor withdrawCollateral validates that _to is not the 0x0 address. They should. Unlike token transfers which typically reject transfers to the 0x0 address, call on the 0x0 address will not fail if the caller has the amount of ETH they send (note this is different from the last issue reported in the QA report, which is regarding contracts only).

Incorrect comment about vault fee.

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/GeneralVault.sol#L51 It is not 20%, it is limited to less than or equal to 30%

Typo in comment

https://github.com/code-423n4/2022-05-sturdy/blob/main/smart-contracts/YieldManager.sol#L217

@param _amount The mount of asset should read @param _amount The amount of asset. amount is spelled incorrectly.

Admin can deceptively increase fees before processing yield

A malicious admin can call setTreasuryInfo immediately before calling processYield to increase the vault fee and then set the vault fee back to the advertised value by calling setTreasuryInfo immediately after. Instead setTreasuryInfo should only be allowed to set the vault fee once. After this, the vault fee should be set when calling processYield for the next time processYield is called.

No need to validate asset is not 0x0 address.

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L71 The call to balanceOf on the next line will fail anyway.