Skip to content

Commit

Permalink
Enforce encumbrances in withdraw and redeem (#17)
Browse files Browse the repository at this point in the history 
We modify `withdraw` and `redeem` to spend encumbrances, then allowances, similar to `transferFrom`.
  • Loading branch information
@kevincheng96
kevincheng96 authored Oct 20, 2023
1 parent 812b43f commit c615df7
Show file tree
Hide file tree
Showing 3 changed files with 226 additions and 40 deletions.
62 changes: 36 additions & 26 deletions src/CometWrapper.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}
31 changes: 17 additions & 14 deletions test/CometWrapper.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -788,35 +790,36 @@ 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);

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);
cometWrapper.approve(alice, type(uint256).max);
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);
Expand Down
173 changes: 173 additions & 0 deletions test/Encumber.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
}
}

0 comments on commit c615df7

Please sign in to comment.