Skip to content

Commit

Permalink
Merge pull request #66 from gnosis/handle-failed-transfer-from-that-d…
Browse files Browse the repository at this point in the history 
…oes-not-revert

[A1] Handle ERC20 tokens that do not revert on failed transferFrom()
  • Loading branch information
@auryn-macmillan
auryn-macmillan authored Nov 21, 2022
2 parents a89329b + f14d338 commit 5f79f71
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 22 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Created under the [LGPL-3.0+ license](LICENSE).

An audit has been performed by the [G0 group](https://github.com/g0-group).

All issues and notes of the audit have been addressed in commit [2341cf0375b8f78b0dc3bd4d0d7ee864e1a6f804](https://github.com/gnosis/zodiac-module-exit/commit/2341cf0375b8f78b0dc3bd4d0d7ee864e1a6f804).
All issues and notes of the audits have been addressed in commit [9fc3b852126520753cca42ee7e1281659e78e9c3](https://github.com/gnosis/zodiac-module-exit/blob/9fc3b852126520753cca42ee7e1281659e78e9c3/packages/contracts/contracts/).

The audit results are available as a pdf in [this repo](packages/contracts/audits/ZodiacExitModuleJan2022.pdf).

Expand Down
Binary file added packages/contracts/audits/ZodiacExitModuleBugFixNov2022.pdf
View file
Binary file not shown.
11 changes: 7 additions & 4 deletions packages/contracts/contracts/ExitModule/ExitERC20Module.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@
pragma solidity ^0.8.6;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

import "./ExitBase.sol";
import "../CirculatingSupply/CirculatingSupplyERC20.sol";

contract ExitERC20 is ExitBase, ReentrancyGuard {
using SafeERC20 for ERC20;

ERC20 public designatedToken;
CirculatingSupplyERC20 public circulatingSupply;

Expand Down Expand Up @@ -43,9 +46,9 @@ contract ExitERC20 is ExitBase, ReentrancyGuard {
address _designatedToken,
address _circulatingSupply
) = abi.decode(
initParams,
(address, address, address, address, address)
);
initParams,
(address, address, address, address, address)
);
__Ownable_init();
require(_avatar != address(0), "Avatar can not be zero address");
require(_target != address(0), "Target can not be zero address");
Expand Down Expand Up @@ -86,7 +89,7 @@ contract ExitERC20 is ExitBase, ReentrancyGuard {
getCirculatingSupply()
);

designatedToken.transferFrom(msg.sender, avatar, amountToRedeem);
designatedToken.safeTransferFrom(msg.sender, avatar, amountToRedeem);

_exit(tokens, params);
}
Expand Down
15 changes: 15 additions & 0 deletions packages/contracts/contracts/test/TestToken.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,19 @@ contract TestToken is ERC20 {
function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
}

// Does not revert on failure
function transferFrom(
address from,
address to,
uint256 amount
) public virtual override returns (bool) {
uint256 allowance = allowance(owner, _msgSender());
if (allowance != type(uint256).max && allowance >= amount) {
_transfer(from, to, amount);
return true;
} else {
return false;
}
}
}
34 changes: 17 additions & 17 deletions packages/contracts/test/01_ExitModule.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ describe("ExitERC20", async () => {
});

describe("setUp() ", () => {
it("throws if module has already been initialized", async () => {
it("reverts if module has already been initialized", async () => {
const { designatedToken, circulatingSupply } = await baseSetup();
const Module = await hre.ethers.getContractFactory("ExitERC20");
const module = await Module.deploy(
Expand All @@ -103,7 +103,7 @@ describe("ExitERC20", async () => {
);
});

it("throws if avatar is zero address", async () => {
it("reverts if avatar is zero address", async () => {
const { designatedToken, circulatingSupply } = await baseSetup();
const Module = await hre.ethers.getContractFactory("ExitERC20");
await expect(
Expand All @@ -117,7 +117,7 @@ describe("ExitERC20", async () => {
).to.be.revertedWith("Avatar can not be zero address");
});

it("throws if target is zero address", async () => {
it("reverts if target is zero address", async () => {
const { designatedToken, circulatingSupply } = await baseSetup();
const Module = await hre.ethers.getContractFactory("ExitERC20");
await expect(
Expand Down Expand Up @@ -160,14 +160,14 @@ describe("ExitERC20", async () => {
const moduleIsAdded = await module.deniedTokens(tokenOne.address);
expect(moduleIsAdded).to.be.true;
});
it("throws if not authorized", async () => {
it("reverts if not authorized", async () => {
const { module, tokenTwo } = await setupTestWithTestAvatar();
await expect(module.addToDenyList([tokenTwo.address])).to.be.revertedWith(
`Ownable: caller is not the owner`
);
});

it("throws if token is already in list", async () => {
it("reverts if token is already in list", async () => {
const { module, avatar, tokenTwo } = await setupTestWithTestAvatar();
const data = module.interface.encodeFunctionData("addToDenyList", [
[tokenTwo.address],
Expand Down Expand Up @@ -200,7 +200,7 @@ describe("ExitERC20", async () => {
expect(moduleIsNotAdded).to.be.false;
});

it("throws if token is not added in list", async () => {
it("reverts if token is not added in list", async () => {
const { module, avatar, tokenTwo } = await setupTestWithTestAvatar();
const removeTokenData = module.interface.encodeFunctionData(
"removeFromDenyList",
Expand All @@ -211,7 +211,7 @@ describe("ExitERC20", async () => {
).to.be.revertedWith(`Token not denied`);
});

it("throws if not authorized", async () => {
it("reverts if not authorized", async () => {
const { module, tokenOne } = await setupTestWithTestAvatar();
await expect(
module.removeFromDenyList([tokenOne.address])
Expand All @@ -220,7 +220,7 @@ describe("ExitERC20", async () => {
});

describe("exit()", () => {
it("throws if token is added in denied tokens list", async () => {
it("reverts if token is added in denied tokens list", async () => {
const { avatar, module, tokenOne, tokenTwo, designatedToken } =
await setupTestWithTestAvatar();
const data = module.interface.encodeFunctionData("addToDenyList", [
Expand All @@ -239,7 +239,7 @@ describe("ExitERC20", async () => {
).to.be.revertedWith(`Denied token`);
});

it("throws if designated token is in list", async () => {
it("reverts if designated token is in list", async () => {
const { avatar, module, tokenOne, designatedToken } =
await setupTestWithTestAvatar();
const data = module.interface.encodeFunctionData("addToDenyList", [
Expand All @@ -255,7 +255,7 @@ describe("ExitERC20", async () => {
).to.be.revertedWith("Designated token can't be redeemed");
});

it("throws because user is trying to redeem more tokens than he owns", async () => {
it("reverts because user is trying to redeem more tokens than he owns", async () => {
const { avatar, module, tokensOrdered } = await setupTestWithTestAvatar();
await avatar.setModule(module.address);
await expect(
Expand Down Expand Up @@ -426,7 +426,7 @@ describe("ExitERC20", async () => {
);
});

it("throws because user haven't approve designated tokens", async () => {
it("reverts if user hasn't approved designated tokens", async () => {
const { avatar, module, tokenOne, tokenTwo } =
await setupTestWithTestAvatar();
await avatar.setModule(module.address);
Expand All @@ -436,7 +436,7 @@ describe("ExitERC20", async () => {
tokenOne.address,
tokenTwo.address,
])
).to.be.revertedWith("ERC20: transfer amount exceeds allowance");
).to.be.revertedWith("SafeERC20: ERC20 operation did not succeed");
});
});

Expand All @@ -459,7 +459,7 @@ describe("ExitERC20", async () => {
expect(newTokenAddress).to.be.equal(tokenOne.address);
});

it("throws if avatar is msg.sender is not the avatar", async () => {
it("reverts if avatar is msg.sender is not the avatar", async () => {
const { module } = await setupTestWithTestAvatar();
await expect(module.setDesignatedToken(AddressZero)).to.be.revertedWith(
"Ownable: caller is not the owner"
Expand All @@ -468,7 +468,7 @@ describe("ExitERC20", async () => {
});

describe("setCirculatingSupply()", () => {
it("throws if avatar is msg.sender is not the owner", async () => {
it("reverts if avatar is msg.sender is not the owner", async () => {
const { module } = await setupTestWithTestAvatar();
await expect(module.setCirculatingSupply(AddressZero)).to.be.revertedWith(
"Ownable: caller is not the owner"
Expand Down Expand Up @@ -501,7 +501,7 @@ describe("ExitERC20", async () => {
expect(newOwner).to.be.equal(AddressZero);
});

it("throws because its not being called by owner", async () => {
it("reverts because its not being called by owner", async () => {
const { module } = await setupTestWithTestAvatar();
await expect(module.renounceOwnership()).to.be.revertedWith(
"Ownable: caller is not the owner"
Expand All @@ -524,7 +524,7 @@ describe("ExitERC20", async () => {
const newAvatar = await module.avatar();
expect(newAvatar).to.be.equal(user.address);
});
it("throws because its not being called by owner", async () => {
it("reverts because its not being called by owner", async () => {
const { module } = await setupTestWithTestAvatar();
await expect(module.setAvatar(user.address)).to.be.revertedWith(
"Ownable: caller is not the owner"
Expand All @@ -548,7 +548,7 @@ describe("ExitERC20", async () => {
expect(newOwner).to.be.equal(user.address);
});

it("throws because its not being called by owner", async () => {
it("reverts because its not being called by owner", async () => {
const { module } = await setupTestWithTestAvatar();
await expect(module.transferOwnership(user.address)).to.be.revertedWith(
"Ownable: caller is not the owner"
Expand Down

1 comment on commit 5f79f71

@vercel
Copy link

@vercel vercel bot commented on 5f79f71 Nov 21, 2022

Choose a reason for hiding this comment

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

Successfully deployed to the following URLs:

zodiac-exit-app – ./

zodiac-exit-app-gnosis-guild.vercel.app
zodiac-exit-app-git-master-gnosis-guild.vercel.app
zodiac-exit-app.vercel.app

Please sign in to comment.