Merge pull request #29 from pooltogether/fix/remove-shares-exploit-fix

fix(contract): remove shares exploit fix
pooltogether · Jul 7, 2022 · d8843c1 · d8843c1
2 parents 8928715 + 05938a7
commit d8843c1
Show file tree

Hide file tree

Showing 3 changed files with 5 additions and 37 deletions.
diff --git a/contracts/yield-source/ATokenYieldSource.sol b/contracts/yield-source/ATokenYieldSource.sol
@@ -229,11 +229,10 @@ contract ATokenYieldSource is ERC20, IProtocolYieldSource, Manageable, Reentranc
     uint256 shares = _tokenToShares(mintAmount);
     _requireSharesGTZero(shares);
 
-    uint256 tokenAmount = _sharesToToken(shares);
-    _depositToAave(tokenAmount);
+    _depositToAave(mintAmount);
     _mint(to, shares);
 
-    emit SuppliedTokenTo(msg.sender, shares, tokenAmount, to);
+    emit SuppliedTokenTo(msg.sender, shares, mintAmount, to);
   }
 
   /// @notice Redeems asset tokens from the yield source
@@ -245,19 +244,17 @@ contract ATokenYieldSource is ERC20, IProtocolYieldSource, Manageable, Reentranc
     uint256 shares = _tokenToShares(redeemAmount);
     _requireSharesGTZero(shares);
 
-    uint256 tokenAmount = _sharesToToken(shares);
-
     _burn(msg.sender, shares);
 
     IERC20 _depositToken = IERC20(_tokenAddress);
     uint256 beforeBalance = _depositToken.balanceOf(address(this));
-    _lendingPool().withdraw(_tokenAddress, tokenAmount, address(this));
+    _lendingPool().withdraw(_tokenAddress, redeemAmount, address(this));
     uint256 afterBalance = _depositToken.balanceOf(address(this));
 
     uint256 balanceDiff = afterBalance.sub(beforeBalance);
     _depositToken.safeTransfer(msg.sender, balanceDiff);
 
-    emit RedeemedToken(msg.sender, shares, tokenAmount);
+    emit RedeemedToken(msg.sender, shares, redeemAmount);
     return balanceDiff;
   }
 

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@pooltogether/aave-yield-source",
-  "version": "1.2.0",
+  "version": "1.2.1",
   "description": "PoolTogether Aave Yield Source",
   "main": "index.js",
   "license": "GPL-3.0",

diff --git a/test/ATokenYieldSource.test.ts b/test/ATokenYieldSource.test.ts
@@ -496,35 +496,6 @@ describe('ATokenYieldSource', () => {
         aTokenYieldSource.connect(attacker).redeemToken(attackerRedeemAmount),
       ).to.be.revertedWith('ATokenYieldSource/shares-gt-zero');
     });
-
-    it('should succeed to manipulate share price but fail to redeem more than deposited', async () => {
-      const amount = toWei('100000');
-      const attackAmount = BigNumber.from(1);
-      const aTokenAmount = toWei('10000');
-
-      await supplyTokenTo(attacker, attackAmount);
-
-      // Attacker sends 10000 aTokens directly to the contract to manipulate share price
-      await aToken.mint(attacker.address, aTokenAmount);
-      await aToken.connect(attacker).approve(aTokenYieldSource.address, aTokenAmount);
-      await aToken.connect(attacker).transfer(aTokenYieldSource.address, aTokenAmount);
-
-      await supplyTokenTo(wallet2, amount);
-
-      const sharePrice = await aTokenYieldSource.sharesToToken(BigNumber.from(1));
-
-      // Redeem 1 wei less than the full amount to burn 1 share instead of 2 because of rounding error
-      // The actual amount of shares to be burnt should be 1.99 but since Solidity truncates down, it will be 1
-      const attackerRedeemAmount = sharePrice.mul(2).sub(1);
-
-      const attackerRedeemShare = await aTokenYieldSource.tokenToShares(attackerRedeemAmount);
-      const redeemAmount = await aTokenYieldSource.sharesToToken(attackerRedeemShare);
-
-      await aTokenYieldSource.connect(attacker).redeemToken(attackerRedeemAmount);
-
-      expect(await usdcToken.balanceOf(attacker.address)).to.equal(redeemAmount);
-      expect(await aTokenYieldSource.balanceOfToken(attacker.address)).to.equal(Zero);
-    });
   });
 
   describe('transferERC20()', () => {