diff --git a/src/CometWrapper.sol b/src/CometWrapper.sol index 6e23412..5bb9cc2 100644 --- a/src/CometWrapper.sol +++ b/src/CometWrapper.sol @@ -145,8 +145,7 @@ contract CometWrapper is ERC4626, IERC7246, CometHelpers { if (shares == 0) revert ZeroShares(); if (msg.sender != owner) { - // TODO: spend encumbrance, then allowance - spendAllowanceInternal(owner, msg.sender, shares); + spendEncumbranceThenAllowanceInternal(owner, msg.sender, shares); } _burn(owner, shares); @@ -168,8 +167,7 @@ contract CometWrapper is ERC4626, IERC7246, CometHelpers { function redeem(uint256 shares, address receiver, address owner) public override returns (uint256) { if (shares == 0) revert ZeroShares(); if (msg.sender != owner) { - // TODO: spend encumbrance, then allowance - spendAllowanceInternal(owner, msg.sender, shares); + spendEncumbranceThenAllowanceInternal(owner, msg.sender, shares); } accrueInternal(owner); @@ -205,24 +203,7 @@ contract CometWrapper is ERC4626, IERC7246, CometHelpers { * @return bool Indicates success of the transfer */ function transferFrom(address from, address to, uint256 amount) public override returns (bool) { - uint256 encumberedToTaker = encumbrances[from][msg.sender]; - if (amount > encumberedToTaker) { - uint256 excessAmount = amount - encumberedToTaker; - - // WARNING: this check needs to happen BEFORE releaseEncumbranceInternal, - // otherwise the released encumbrance will increase availableBalanceOf(from), - // allowing msg.sender to transfer tokens that are encumbered to someone else - - if (availableBalanceOf(from) < excessAmount) revert InsufficientAvailableBalance(); - - // Exceeds Encumbrance, so spend all of it - releaseEncumbranceInternal(from, msg.sender, encumberedToTaker); - - spendAllowanceInternal(from, msg.sender, excessAmount); - } else { - releaseEncumbranceInternal(from, msg.sender, amount); - } - + spendEncumbranceThenAllowanceInternal(from, msg.sender, amount); transferInternal(from, to, amount); return true; } @@ -488,7 +469,7 @@ contract CometWrapper is ERC4626, IERC7246, CometHelpers { * @dev The current timestamp * From https://github.com/compound-finance/comet/blob/main/contracts/Comet.sol#L375-L378 */ - function getNowInternal() internal view virtual returns (uint40) { + function getNowInternal() internal view returns (uint40) { if (block.timestamp >= 2**40) revert TimestampTooLarge(); return uint40(block.timestamp); } @@ -505,7 +486,7 @@ contract CometWrapper is ERC4626, IERC7246, CometHelpers { address owner, address spender, uint256 amount - ) internal virtual { + ) internal { uint256 allowed = allowance[owner][spender]; if (allowed < amount) revert InsufficientAllowance(); if (allowed != type(uint256).max) { @@ -538,7 +519,7 @@ contract CometWrapper is ERC4626, IERC7246, CometHelpers { /** * @dev Increase `owner`'s encumbrance to `taker` by `amount` */ - function encumberInternal(address owner, address taker, uint256 amount) private { + function encumberInternal(address owner, address taker, uint256 amount) internal { if (availableBalanceOf(owner) < amount) revert InsufficientAvailableBalance(); encumbrances[owner][taker] += amount; encumberedBalanceOf[owner] += amount; @@ -574,10 +555,39 @@ contract CometWrapper is ERC4626, IERC7246, CometHelpers { /** * @dev Reduce `owner`'s encumbrance to `taker` by `amount` */ - function releaseEncumbranceInternal(address owner, address taker, uint256 amount) private { + function releaseEncumbranceInternal(address owner, address taker, uint256 amount) internal { if (encumbrances[owner][taker] < amount) revert InsufficientEncumbrance(); encumbrances[owner][taker] -= amount; encumberedBalanceOf[owner] -= amount; emit Release(owner, taker, amount); } + + /** + * @notice Spends an amount of an `owner`'s encumbrance to `spender`, falling back to their allowance for any + * amount not covered by the encumbrance + * @param owner The address that encumbrances and allowances are spent from + * @param spender The address that is spending the encumbrance and allowance + * @param amount The amount of encumbrance and/or allowance to be spent + */ + function spendEncumbranceThenAllowanceInternal(address owner, address spender, uint256 amount) internal { + uint256 encumberedToTaker = encumbrances[owner][spender]; + if (amount > encumberedToTaker) { + uint256 excessAmount = amount - encumberedToTaker; + + // WARNING: This check needs to happen BEFORE releaseEncumbranceInternal, + // otherwise the released encumbrance will increase availableBalanceOf(from), + // allowing msg.sender to transfer tokens that are encumbered to someone else + + // Check to make sure that the owner has enough available balance to move around + // so as not to move tokens encumbered to others + if (availableBalanceOf(owner) < excessAmount) revert InsufficientAvailableBalance(); + + // Exceeds Encumbrance, so spend all of it + releaseEncumbranceInternal(owner, spender, encumberedToTaker); + + spendAllowanceInternal(owner, spender, excessAmount); + } else { + releaseEncumbranceInternal(owner, spender, amount); + } + } } diff --git a/test/CometWrapper.t.sol b/test/CometWrapper.t.sol index d346911..04cd4c8 100644 --- a/test/CometWrapper.t.sol +++ b/test/CometWrapper.t.sol @@ -536,7 +536,7 @@ abstract contract CometWrapperTest is CoreTest, CometMath { assertLe(comet.balanceOf(alice), expectedAliceCometBalance); } - function test_withdrawUsesAllowances() public { + function test_withdrawFromUsesAllowances() public { setUpAliceAndBobCometBalances(); vm.startPrank(alice); @@ -550,8 +550,8 @@ abstract contract CometWrapperTest is CoreTest, CometMath { vm.stopPrank(); uint256 sharesToApprove = 2_700 * decimalScale; - uint256 sharesToWithdraw = 2_500 * decimalScale; - uint256 assetsToWithdraw = cometWrapper.previewRedeem(sharesToWithdraw); + uint256 sharesToRedeem = 2_500 * decimalScale; + uint256 assetsToWithdraw = cometWrapper.previewRedeem(sharesToRedeem); vm.prank(alice); cometWrapper.approve(bob, sharesToApprove); @@ -560,13 +560,15 @@ abstract contract CometWrapperTest is CoreTest, CometMath { // Allowances should be updated when withdraw is done assertEq(cometWrapper.allowance(alice, bob), sharesToApprove); cometWrapper.withdraw(assetsToWithdraw, bob, alice); - assertEq(cometWrapper.balanceOf(alice), 5_000 * decimalScale - sharesToWithdraw); + assertEq(cometWrapper.allowance(alice, bob), sharesToApprove - sharesToRedeem); + assertEq(cometWrapper.balanceOf(alice), 5_000 * decimalScale - sharesToRedeem); // Reverts if trying to withdraw again now that allowance is used up + assetsToWithdraw = cometWrapper.previewRedeem(sharesToRedeem); vm.expectRevert(CometWrapper.InsufficientAllowance.selector); cometWrapper.withdraw(assetsToWithdraw, bob, alice); vm.stopPrank(); - assertEq(cometWrapper.allowance(alice, bob), sharesToApprove - sharesToWithdraw); + assertEq(cometWrapper.allowance(alice, bob), sharesToApprove - sharesToRedeem); // Infinite allowance does not decrease allowance vm.prank(bob); @@ -579,7 +581,7 @@ abstract contract CometWrapperTest is CoreTest, CometMath { vm.stopPrank(); } - function test_withdraw_revertsOnInsufficientAllowance() public { + function test_withdrawFrom_revertsOnInsufficientAllowance() public { setUpAliceAndBobCometBalances(); vm.startPrank(alice); @@ -774,7 +776,7 @@ abstract contract CometWrapperTest is CoreTest, CometMath { assertLe(comet.balanceOf(alice), expectedAliceCometBalance); } - function test_redeemUsesAllowances() public { + function test_redeemFromUsesAllowances() public { setUpAliceAndBobCometBalances(); vm.startPrank(alice); @@ -788,7 +790,7 @@ abstract contract CometWrapperTest is CoreTest, CometMath { vm.stopPrank(); uint256 sharesToApprove = 2_700 * decimalScale; - uint256 sharesToWithdraw = 2_500 * decimalScale; + uint256 sharesToRedeem = 2_500 * decimalScale; vm.prank(alice); cometWrapper.approve(bob, sharesToApprove); @@ -796,14 +798,15 @@ abstract contract CometWrapperTest is CoreTest, CometMath { vm.startPrank(bob); // Allowances should be updated when redeem is done assertEq(cometWrapper.allowance(alice, bob), sharesToApprove); - cometWrapper.redeem(sharesToWithdraw, bob, alice); - assertApproxEqAbs(cometWrapper.balanceOf(alice), 5_000 * decimalScale - sharesToWithdraw, 1); + cometWrapper.redeem(sharesToRedeem, bob, alice); + assertEq(cometWrapper.allowance(alice, bob), sharesToApprove - sharesToRedeem); + assertEq(cometWrapper.balanceOf(alice), 5_000 * decimalScale - sharesToRedeem); // Reverts if trying to redeem again now that allowance is used up vm.expectRevert(CometWrapper.InsufficientAllowance.selector); - cometWrapper.redeem(sharesToWithdraw, bob, alice); + cometWrapper.redeem(sharesToRedeem, bob, alice); vm.stopPrank(); - assertEq(cometWrapper.allowance(alice, bob), sharesToApprove - sharesToWithdraw); + assertEq(cometWrapper.allowance(alice, bob), sharesToApprove - sharesToRedeem); // Infinite allowance does not decrease allowance vm.prank(bob); @@ -811,12 +814,12 @@ abstract contract CometWrapperTest is CoreTest, CometMath { assertEq(cometWrapper.allowance(bob, alice), type(uint256).max); vm.startPrank(alice); - cometWrapper.redeem(sharesToWithdraw, alice, bob); + cometWrapper.redeem(sharesToRedeem, alice, bob); assertEq(cometWrapper.allowance(bob, alice), type(uint256).max); vm.stopPrank(); } - function test_redeem_revertsOnInsufficientAllowance() public { + function test_redeemFrom_revertsOnInsufficientAllowance() public { setUpAliceAndBobCometBalances(); vm.startPrank(alice); diff --git a/test/Encumber.t.sol b/test/Encumber.t.sol index 0543a30..a965e4d 100644 --- a/test/Encumber.t.sol +++ b/test/Encumber.t.sol @@ -7,6 +7,17 @@ abstract contract EncumberTest is CoreTest { event Encumber(address indexed owner, address indexed taker, uint amount); event Release(address indexed owner, address indexed taker, uint amount); + function setUpAliceCometBalance() public { + deal(address(underlyingToken), address(cometHolder), 20_000 * decimalScale); + vm.startPrank(cometHolder); + underlyingToken.approve(address(comet), 20_000 * decimalScale); + comet.supply(address(underlyingToken), 20_000 * decimalScale); + + comet.transfer(alice, 10_000 * decimalScale); + assertGt(comet.balanceOf(alice), 9999 * decimalScale); + vm.stopPrank(); + } + function test_availableBalanceOf() public { vm.startPrank(alice); @@ -319,4 +330,166 @@ abstract contract EncumberTest is CoreTest { assertEq(cometWrapper.encumberedBalanceOf(alice), 100e18); assertEq(cometWrapper.encumbrances(alice, bob), 100e18); } + + /* ===== ERC4626 + Encumbrance Tests ===== */ + + function test_withdrawFromUsesOnlyEncumbrance() public { + setUpAliceCometBalance(); + + vm.startPrank(alice); + comet.allow(wrapperAddress, true); + cometWrapper.mint(5_000 * decimalScale, alice); + vm.stopPrank(); + + uint256 sharesToEncumber = 2_700 * decimalScale; + uint256 sharesToRedeem = 2_500 * decimalScale; + uint256 assetsToWithdraw = cometWrapper.previewRedeem(sharesToRedeem); + + vm.prank(alice); + cometWrapper.encumber(bob, sharesToEncumber); + + vm.startPrank(bob); + // Encumbrance should be updated when withdraw is done + assertEq(cometWrapper.encumbrances(alice, bob), sharesToEncumber); + cometWrapper.withdraw(assetsToWithdraw, bob, alice); + assertEq(cometWrapper.encumbrances(alice, bob), sharesToEncumber - sharesToRedeem); + assertEq(cometWrapper.balanceOf(alice), 5_000 * decimalScale - sharesToRedeem); + + // Reverts if trying to withdraw again now that encumbrance is used up + assetsToWithdraw = cometWrapper.previewRedeem(sharesToRedeem); + vm.expectRevert(CometWrapper.InsufficientAllowance.selector); + cometWrapper.withdraw(assetsToWithdraw, bob, alice); + vm.stopPrank(); + assertEq(cometWrapper.encumbrances(alice, bob), sharesToEncumber - sharesToRedeem); + } + + function test_withdrawFromUsesEncumbranceAndAllowance() public { + setUpAliceCometBalance(); + + vm.startPrank(alice); + comet.allow(wrapperAddress, true); + cometWrapper.mint(5_000 * decimalScale, alice); + vm.stopPrank(); + + uint256 sharesToEncumber = 1_000 * decimalScale; + uint256 sharesToApprove = 1_700 * decimalScale; + uint256 sharesToRedeem = 2_500 * decimalScale; + uint256 assetsToWithdraw = cometWrapper.previewRedeem(sharesToRedeem); + + vm.startPrank(alice); + cometWrapper.encumber(bob, sharesToEncumber); + cometWrapper.approve(bob, sharesToApprove); + vm.stopPrank(); + + vm.startPrank(bob); + // Encumbrance and allowance should be updated when withdraw is done + assertEq(cometWrapper.encumbrances(alice, bob), sharesToEncumber); + assertEq(cometWrapper.allowance(alice, bob), sharesToApprove); + cometWrapper.withdraw(assetsToWithdraw, bob, alice); + assertEq(cometWrapper.encumbrances(alice, bob), 0); + assertEq(cometWrapper.allowance(alice, bob), sharesToEncumber + sharesToApprove - sharesToRedeem); + assertEq(cometWrapper.balanceOf(alice), 5_000 * decimalScale - sharesToRedeem); + + // Reverts if trying to withdraw again now that encumbrance and allowance is used up + assetsToWithdraw = cometWrapper.previewRedeem(sharesToRedeem); + vm.expectRevert(CometWrapper.InsufficientAllowance.selector); + cometWrapper.withdraw(assetsToWithdraw, bob, alice); + vm.stopPrank(); + assertEq(cometWrapper.encumbrances(alice, bob), 0); + assertEq(cometWrapper.allowance(alice, bob), sharesToEncumber + sharesToApprove - sharesToRedeem); + } + + function test_withdrawFrom_revertsOnInsufficientAvailableBalance() public { + setUpAliceCometBalance(); + + vm.startPrank(alice); + comet.allow(wrapperAddress, true); + cometWrapper.deposit(1_000 * decimalScale, alice); + // Encumber to Charlie so Alice's available balance is only 100 + cometWrapper.encumber(charlie, 900 * decimalScale); + cometWrapper.approve(bob, 200 * decimalScale); + vm.stopPrank(); + + vm.prank(bob); + vm.expectRevert(CometWrapper.InsufficientAvailableBalance.selector); + cometWrapper.withdraw(200 * decimalScale, bob, alice); + } + + function test_redeemFromUsesOnlyEncumbrance() public { + setUpAliceCometBalance(); + + vm.startPrank(alice); + comet.allow(wrapperAddress, true); + cometWrapper.mint(5_000 * decimalScale, alice); + vm.stopPrank(); + + uint256 sharesToEncumber = 2_700 * decimalScale; + uint256 sharesToRedeem = 2_500 * decimalScale; + + vm.prank(alice); + cometWrapper.encumber(bob, sharesToEncumber); + + vm.startPrank(bob); + // Encumbrances should be updated when redeem is done + assertEq(cometWrapper.encumbrances(alice, bob), sharesToEncumber); + cometWrapper.redeem(sharesToRedeem, bob, alice); + assertEq(cometWrapper.encumbrances(alice, bob), sharesToEncumber - sharesToRedeem); + assertEq(cometWrapper.balanceOf(alice), 5_000 * decimalScale - sharesToRedeem); + + // Reverts if trying to redeem again now that encumbrance is used up + vm.expectRevert(CometWrapper.InsufficientAllowance.selector); + cometWrapper.redeem(sharesToRedeem, bob, alice); + vm.stopPrank(); + assertEq(cometWrapper.encumbrances(alice, bob), sharesToEncumber - sharesToRedeem); + } + + function test_redeemFromUsesEncumbranceAndAllowance() public { + setUpAliceCometBalance(); + + vm.startPrank(alice); + comet.allow(wrapperAddress, true); + cometWrapper.mint(5_000 * decimalScale, alice); + vm.stopPrank(); + + uint256 sharesToEncumber = 1_000 * decimalScale; + uint256 sharesToApprove = 1_700 * decimalScale; + uint256 sharesToRedeem = 2_500 * decimalScale; + + vm.startPrank(alice); + cometWrapper.encumber(bob, sharesToEncumber); + cometWrapper.approve(bob, sharesToApprove); + vm.stopPrank(); + + vm.startPrank(bob); + // Encumbrances and allowances should be updated when redeem is done + assertEq(cometWrapper.encumbrances(alice, bob), sharesToEncumber); + assertEq(cometWrapper.allowance(alice, bob), sharesToApprove); + cometWrapper.redeem(sharesToRedeem, bob, alice); + assertEq(cometWrapper.encumbrances(alice, bob), 0); + assertEq(cometWrapper.allowance(alice, bob), sharesToEncumber + sharesToApprove - sharesToRedeem); + assertEq(cometWrapper.balanceOf(alice), 5_000 * decimalScale - sharesToRedeem); + + // Reverts if trying to redeem again now that encumbrance and allowance is used up + vm.expectRevert(CometWrapper.InsufficientAllowance.selector); + cometWrapper.redeem(sharesToRedeem, bob, alice); + vm.stopPrank(); + assertEq(cometWrapper.encumbrances(alice, bob), 0); + assertEq(cometWrapper.allowance(alice, bob), sharesToEncumber + sharesToApprove - sharesToRedeem); + } + + function test_redeemFrom_revertsOnInsufficientAvailableBalance() public { + setUpAliceCometBalance(); + + vm.startPrank(alice); + comet.allow(wrapperAddress, true); + cometWrapper.mint(1_000 * decimalScale, alice); + // Encumber to Charlie so Alice's available balance is only 100 + cometWrapper.encumber(charlie, 900 * decimalScale); + cometWrapper.approve(bob, 200 * decimalScale); + vm.stopPrank(); + + vm.prank(bob); + vm.expectRevert(CometWrapper.InsufficientAvailableBalance.selector); + cometWrapper.redeem(200 * decimalScale, bob, alice); + } }