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.

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

Fix spend allowance overloading #368

Conversation

pedrovalido
Copy link
Contributor

@pedrovalido pedrovalido commented Mar 13, 2023

This pull request addresses C-1 of Macro audit report

@pedrovalido pedrovalido self-assigned this Mar 13, 2023
@pedrovalido pedrovalido linked an issue Mar 13, 2023 that may be closed by this pull request
@pedrovalido pedrovalido marked this pull request as ready for review March 14, 2023 01:39
@0xdcota 0xdcota added this to the Fuji-v2 MVP milestone Mar 20, 2023
@0xdcota 0xdcota changed the base branch from main to protocol/fix/macro-findings-critical-high March 22, 2023 20:27
@0xdcota 0xdcota merged commit 8851946 into protocol/fix/macro-findings-critical-high Mar 22, 2023
@0xdcota 0xdcota deleted the protocol/fix/spend-allowance-overloading branch March 22, 2023 20:27
* @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.

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

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

@0xcuriousapple
Copy link
Collaborator

Next PR Made to resolve issues of this PR: #536

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect overloading of _spendAllowance
3 participants