code-423n4 / 2022-11-redactedcartel-findings

3 stars 2 forks source link

Possible funds lost in `PirexERC6426` due to missing address check #281

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L53 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L99 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L124 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L238 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L256

Vulnerability details

Impact

PirexERC4626::redeem, PirexERC4626::transfer, PirexERC4626::transferFrom, PirexERC4626:constructor and PirexERC4626::withdraw does not check if address to interact with is not zero. Not checking it may lead to funds lost permemently.

Proof of Concept

  1. User calls impacted function iwth default receiver (address 0)
  2. All funds are sent to address 0, hence lost

Tools Used

VS Code

Recommended Mitigation Steps

Add check for 0 address, and for 0 amount, to save gas

diff --git a/src/vaults/PirexERC4626.sol b/src/vaults/PirexERC4626.sol
index 90c4493..677fdc8 100644
--- a/src/vaults/PirexERC4626.sol
+++ b/src/vaults/PirexERC4626.sol
@@ -39,6 +39,8 @@ abstract contract PirexERC4626 is ERC20 {
         uint256 shares
     );

+    error ZeroAddress();
+
     /*//////////////////////////////////////////////////////////////
                                IMMUTABLES
     //////////////////////////////////////////////////////////////*/
@@ -50,6 +52,7 @@ abstract contract PirexERC4626 is ERC20 {
         string memory _name,
         string memory _symbol
     ) ERC20(_name, _symbol, _asset.decimals()) {
+        if (address(_asset) == address(0)) revert ZeroAddress();
         asset = _asset;
     }

@@ -62,6 +65,8 @@ abstract contract PirexERC4626 is ERC20 {
         virtual
         returns (uint256 shares)
     {
+        if (receiver == address(0)) revert ZeroAddress();
+
         if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

         // Check for rounding error since we round down in previewDeposit.
@@ -82,6 +87,8 @@ abstract contract PirexERC4626 is ERC20 {
         virtual
         returns (uint256 assets)
     {
+        if (receiver == address(0)) revert ZeroAddress();
+
         if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

         assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.
@@ -102,6 +109,7 @@ abstract contract PirexERC4626 is ERC20 {
         address owner
     ) public virtual returns (uint256 shares) {
         shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.
+        if (receiver == address(0)) revert ZeroAddress();

         if (msg.sender != owner) {
             uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.
@@ -126,6 +134,8 @@ abstract contract PirexERC4626 is ERC20 {
         address receiver,
         address owner
     ) public virtual returns (uint256 assets) {
+        if (receiver == address(0)) revert ZeroAddress();
+
         if (msg.sender != owner) {
             uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

@@ -240,6 +250,8 @@ abstract contract PirexERC4626 is ERC20 {
         override
         returns (bool)
     {
+        if (to == address(0)) revert ZeroAddress();
+
         bool status = ERC20.transfer(to, amount);

         afterTransfer(msg.sender, to, amount);
@@ -258,6 +270,8 @@ abstract contract PirexERC4626 is ERC20 {
         address to,
         uint256 amount
     ) public override returns (bool) {
+        if (to == address(0)) revert ZeroAddress();
+
         bool status = ERC20.transferFrom(from, to, amount);

         afterTransfer(from, to, amount);
c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-11-redactedcartel-findings/issues/282