Fix for etherwrapper rounding/precision issue (#1261)

* wip fix for etherwrapper rounding issues

* wip: maybe a fix?

* precision test is failing correctly

* fix precision issue

* refactor: simplify calculation

Author: liamzebedee

contracts/EtherWrapper.sol

@@ -166,12 +166,12 @@ contract EtherWrapper is Owned, Pausable, MixinResolver, MixinSystemSettings, IE
         require(reserves > 0, "Contract cannot burn sETH for WETH, WETH balance is zero");
 
         // principal = [amountIn / (1 + burnFeeRate)]
-        uint principal = amountIn.divideDecimal(SafeDecimalMath.unit().add(burnFeeRate()));
+        uint principal = amountIn.divideDecimalRound(SafeDecimalMath.unit().add(burnFeeRate()));
 
         if (principal < reserves) {
-            _burn(principal);
+            _burn(principal, amountIn);
         } else {
-            _burn(reserves);
+            _burn(reserves, reserves.add(calculateBurnFee(reserves)));
         }
     }
 
@@ -227,10 +227,9 @@ contract EtherWrapper is Owned, Pausable, MixinResolver, MixinSystemSettings, IE
         emit Minted(msg.sender, principal, feeAmountEth, amountIn);
     }
 
-    function _burn(uint principal) internal {
+    function _burn(uint principal, uint amountIn) internal {
         // for burn, amount is inclusive of the fee.
-        uint feeAmountEth = calculateBurnFee(principal);
-        uint amountIn = principal.add(feeAmountEth);
+        uint feeAmountEth = amountIn.sub(principal);
 
         require(amountIn <= IERC20(address(synthsETH())).allowance(msg.sender, address(this)), "Allowance not high enough");
         require(amountIn <= IERC20(address(synthsETH())).balanceOf(msg.sender), "Balance is too low");

test/contracts/EtherWrapper.js

@@ -482,6 +482,37 @@ contract('EtherWrapper', async accounts => {
 					assert.equal(await etherWrapper.getReserves(), '0');
 				});
 			});
+
+			describe('precision and rounding', async () => {
+				let burnAmount;
+				let burnTx;
+
+				before(async () => {
+					const amount = toUnit('1.2');
+					await weth.deposit({ from: account1, value: amount });
+					await weth.approve(etherWrapper.address, amount, { from: account1 });
+					await etherWrapper.mint(amount, { from: account1 });
+
+					burnAmount = toUnit('0.9');
+					await sETHSynth.issue(account1, burnAmount);
+					await sETHSynth.approve(etherWrapper.address, burnAmount, { from: account1 });
+					burnTx = await etherWrapper.burn(burnAmount, { from: account1 });
+				});
+				it('emits a Burn event which burns 0.9 sETH', async () => {
+					const logs = await getDecodedLogs({
+						hash: burnTx.tx,
+						contracts: [sETHSynth],
+					});
+
+					decodedEventEqual({
+						event: 'Burned',
+						emittedFrom: sETHSynth.address,
+						args: [account1, burnAmount],
+						log: logs.filter(l => !!l).find(({ name }) => name === 'Burned'),
+						bnCloseVariance: 0,
+					});
+				});
+			});
 		});
 	});