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

Vault inflation attack #498

Closed

Conversation

0xdcota
Copy link
Contributor

@0xdcota 0xdcota commented Apr 26, 2023

This pull request addresses vulnerabilities M-1 of Macro audit report.

Base automatically changed from protocol/fix/vault-debt-function-completeness-and-rounding to protocol/fix/macro-findings-medium May 2, 2023 21:09
Copy link
Contributor

@pedrovalido pedrovalido left a comment

Choose a reason for hiding this comment

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

Friendly reminder about the TODO's

@pedrovalido
Copy link
Contributor

Lets have a brainstorm session to try and attack the newly implemented fix. Action to consider:

  • deposit
  • withdraw
  • payback
  • borrow
  • transfer
  • burn
  • malicious providers
  • etc (open to more suggetions)

@daigarocota

Copy link
Contributor

@pedrovalido pedrovalido left a comment

Choose a reason for hiding this comment

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

When a certain amount of time passes and the vault gains interest, the vault will steal the user's interest.
Flow:
Vault has 100 shares in the beginning.
Vault has 100 fake assets in the beginning.
User deposits 100 assets
User get 100 shares
Sometime passes and the user's money gains 10 assets of interest.

User's balance would be 105 instead of 110

This could go either way, for collateral or for debt

Copy link
Contributor

@pedrovalido pedrovalido left a comment

Choose a reason for hiding this comment

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

When calling the deposit function with the vault's address as the receiver, those funds will get stuck in the vault

@pedrovalido
Copy link
Contributor

pedrovalido commented May 5, 2023

Possible solution: when deploying a vault, use the initializeVaultShares function to also deposit a certain amount of real assets instead of faking. Do the same for debt.

Comment on lines +241 to +251
/**
* @dev Extended to avoid burning of address(this) shares created in
* `_initializeVaultShares`. See OZ implementation: {ERC20-_burn}.
*/
function __burn(address account, uint256 amount) internal virtual {
if (account == address(this)) {
revert BaseVault__burn_cannotBurn();
}
_burn(account, amount);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this. Its not going to be used

@0xdcota
Copy link
Contributor Author

0xdcota commented May 22, 2023

This pull request was overriden by #541 .

@0xdcota 0xdcota closed this May 22, 2023
@brozorec brozorec deleted the protocol/fix/vault-inflation branch June 21, 2023 09:36
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.

M-1 Remaining inflation attack
2 participants