Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix spend allowance overloading #368

Merged
merged 2 commits into from Mar 22, 2023
Merged

Fix spend allowance overloading #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
88a5ac8
fix(protocol): small typo
pedrovalido Mar 13, 2023
6e003f8
fix(protocol): spend allowance not called by transfer
pedrovalido Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 15 additions & 1 deletion packages/protocol/src/abstracts/BaseVault.sol
View file
Open in desktop
Expand Up @@ -187,7 +187,7 @@ abstract contract BaseVault is ERC20, SystemAccessControl, PausableVault, VaultP
* @dev Called during {ERC20-transferFrom} to decrease allowance.
* Requirements:
* - Must be overriden to call {VaultPermissions-_spendWithdrawAllowance}.
* - Msut convert `shares` to `assets` before calling internal functions.
* - Must convert `shares` to `assets` before calling internal functions.
*
* @param owner of `shares`
* @param operator allowed to act on `owners` behalf
Expand All @@ -205,6 +205,20 @@ abstract contract BaseVault is ERC20, SystemAccessControl, PausableVault, VaultP
_spendWithdrawAllowance(owner, operator, receiver, convertToAssets(shares));
}

/**
* @dev Called during {ERC20-transferFrom} to decrease allowance.
* Requirements:
* - Must be overriden to call {VaultPermissions-_spendWithdrawAllowance}.
* - Must convert `shares` to `assets` before calling internal functions.
*
* @param owner of `shares`
* @param receiver to whom `shares` will be spent
* @param shares amount to spend
*/
function _spendAllowance(address owner, address receiver, uint256 shares) internal override {
_spendWithdrawAllowance(owner, receiver, receiver, convertToAssets(shares));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fuji contracts allow users to increase/decrease their withdrawal allowance per operator, per receiver through various methods of vault permissions.

fuji-v2/packages/protocol/src/vaults/VaultPermissions.sol

Lines 94 to 108 in efed334

function increaseWithdrawAllowance(
address operator,
address receiver,
uint256 byAmount
)
public
override
returns (bool)
{
address owner = msg.sender;
_setWithdrawAllowance(
owner, operator, receiver, _withdrawAllowance[owner][operator][receiver] + byAmount
);
return true;
}

Hence it's not always given that receiver == operator
Which contradicts this PR

If an allowance is set for a different receiver than the operator, this pr would always spend the allowance for the same operator only.
Either consider restricting operator= receiver only in all allowance methods or refractor this fix to accommodate operator!=receiver case.
One way of doing the latter is overriding transferFrom method directly and using _spendWithdrawAllowance in place of _spendAllowance
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1642b6639b93e3b97be163d49827e1f56b81ca11/contracts/token/ERC20/ERC20.sol#L160

owner is from
spender is operator
to is receiver
amount needs to be converted to assets

Copy link
Contributor

@0xdcota 0xdcota May 17, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is three ways to modify the '_withdrawAllowance` state defined in the VaultPermissions contract:

  1. The ERC20 approve method, required by EIP20, that we override at the BaseVault level. Approve assumes operator = receiver because we have no option. You cannot change the approve method signature and we don't want to have a "dual" approval system. We need to force all approvals to be handled by '_withdrawAllowance` state defined in the VaultPermissions contract.
  2. The ERC20 implementation from openzeppelin, has the increaseAllowance or decreaseAllowance which we override at the BaseVault so that, again, there is no "dual" approval system. Since the signature is already defined we then have to assume operator = receiver. If there was a way to delete methods from a parent contract we would just delete these two. They are not official part of the EIP20.
  3. The VaultPermissions contract has method increaseWithdrawAllowance or decreaseWithdrawAllowance which includes the parameter sperately for operator and receiver. This is what we want users to "default".

I hope this clarifies. In the mean time I did some small refactors, including to consider the msg.sender in transferFrom as the "operator". Refer to changes in #536

}

/*//////////////////////////////////////////
Asset management: overrides IERC4626
//////////////////////////////////////////*/
Expand Down
33 changes: 33 additions & 0 deletions packages/protocol/test/mocking/vaults/VaultPermissions.t.sol
View file
Open in desktop
Expand Up @@ -479,4 +479,37 @@ contract VaultPermissionsUnitTests is MockingSetup, MockRoutines {
wrongPermit.owner, wrongPermit.receiver, wrongPermit.amount, wrongPermit.deadline, v, r, s
);
}

function testFail_spendAllowanceIssue() public {
do_deposit(1 ether, vault, owner);

vm.startPrank(owner);
vault.approve(receiver, 1 ether);
uint256 allowance1 = vault.allowance(owner, receiver);
vm.stopPrank();

vm.startPrank(receiver);
vault.transferFrom(owner, receiver, 1 ether);
uint256 allowance2 = vault.allowance(owner, receiver);
vm.stopPrank();

assertEq(allowance1, allowance2);
}

function test_spendAllowanceIssue() public {
do_deposit(1 ether, vault, owner);

vm.startPrank(owner);
vault.approve(receiver, 1 ether);
uint256 allowance1 = vault.allowance(owner, receiver);
vm.stopPrank();

vm.startPrank(receiver);
vault.transferFrom(owner, receiver, 1 ether);
uint256 allowance2 = vault.allowance(owner, receiver);
vm.stopPrank();

assertEq(allowance1, 1 ether);
assertEq(allowance2, 0);
}
}