Skip to content

Commit

Permalink
Use the asset getter in totalAssets, _deposit and _withdraw i…
Browse files Browse the repository at this point in the history
…n ERC4626 (#5322)
  • Loading branch information
ernestognw authored Dec 19, 2024
1 parent e8f24d6 commit 3b240d7
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 5 deletions.
5 changes: 5 additions & 0 deletions .changeset/thin-eels-cross.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': patch
---

`ERC4626`: Use the `asset` getter in `totalAssets`, `_deposit` and `_withdraw`.
10 changes: 5 additions & 5 deletions contracts/token/ERC20/extensions/ERC4626.sol
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ abstract contract ERC4626 is ERC20, IERC4626 {

/** @dev See {IERC4626-totalAssets}. */
function totalAssets() public view virtual returns (uint256) {
return _asset.balanceOf(address(this));
return IERC20(asset()).balanceOf(address(this));
}

/** @dev See {IERC4626-convertToShares}. */
Expand Down Expand Up @@ -237,14 +237,14 @@ abstract contract ERC4626 is ERC20, IERC4626 {
* @dev Deposit/mint common workflow.
*/
function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal virtual {
// If _asset is ERC-777, `transferFrom` can trigger a reentrancy BEFORE the transfer happens through the
// If asset() is ERC-777, `transferFrom` can trigger a reentrancy BEFORE the transfer happens through the
// `tokensToSend` hook. On the other hand, the `tokenReceived` hook, that is triggered after the transfer,
// calls the vault, which is assumed not malicious.
//
// Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
// assets are transferred and before the shares are minted, which is a valid state.
// slither-disable-next-line reentrancy-no-eth
SafeERC20.safeTransferFrom(_asset, caller, address(this), assets);
SafeERC20.safeTransferFrom(IERC20(asset()), caller, address(this), assets);
_mint(receiver, shares);

emit Deposit(caller, receiver, assets, shares);
Expand All @@ -264,14 +264,14 @@ abstract contract ERC4626 is ERC20, IERC4626 {
_spendAllowance(owner, caller, shares);
}

// If _asset is ERC-777, `transfer` can trigger a reentrancy AFTER the transfer happens through the
// If asset() is ERC-777, `transfer` can trigger a reentrancy AFTER the transfer happens through the
// `tokensReceived` hook. On the other hand, the `tokensToSend` hook, that is triggered before the transfer,
// calls the vault, which is assumed not malicious.
//
// Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
// shares are burned and after the assets are transferred, which is a valid state.
_burn(owner, shares);
SafeERC20.safeTransfer(_asset, receiver, assets);
SafeERC20.safeTransfer(IERC20(asset()), receiver, assets);

emit Withdraw(caller, receiver, owner, assets, shares);
}
Expand Down

0 comments on commit 3b240d7

Please sign in to comment.