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.

Sign up for GitHub

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

address audit feedback #120

Merged
merged 12 commits into from
Jan 30, 2024
Merged

address audit feedback #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a8706b6
fix BGB-03
gianchandania Jan 23, 2024
9305ad5
fix RSC-02
gianchandania Jan 23, 2024
ccad0a3
fix BGB-02
gianchandania Jan 23, 2024
c663442
fix GLOBAL-01
gianchandania Jan 23, 2024
6fe7567
fix WSG-03
gianchandania Jan 23, 2024
0d97e38
fix BGB-01
gianchandania Jan 23, 2024
5b81368
fix WSG-02
gianchandania Jan 24, 2024
c46a116
fix WSG-01
gianchandania Jan 24, 2024
dc977af
fix BAT-02
gianchandania Jan 25, 2024
ac1ec04
fix BAC-01
gianchandania Jan 25, 2024
fed562a
fix WSG-06
gianchandania Jan 25, 2024
848a4cd
update optimizer runs
gianchandania Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 12 additions & 24 deletions contracts/Batcher.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pragma solidity 0.8.20;
import '@openzeppelin/contracts/access/Ownable2Step.sol';

// SPDX-License-Identifier: Apache-2.0

Expand All @@ -11,28 +12,24 @@ pragma solidity 0.8.20;
* funnel off those funds to the correct accounts in a single transaction. This is useful for saving on gas when a
* bunch of funds need to be transferred to different accounts.
*
* If more ETH is sent to `batch` than it is instructed to transfer, contact the contract owner in order to recover the excess.
* If more ETH is sent to `batch` than it is instructed to transfer, then the entire transaction will revert
* If any tokens are accidentally transferred to this account, contact the contract owner in order to recover them.
*
*/

contract Batcher {
contract Batcher is Ownable2Step {
event BatchTransfer(address sender, address recipient, uint256 value);
event OwnerChange(address prevOwner, address newOwner);
event TransferGasLimitChange(
uint256 prevTransferGasLimit,
uint256 newTransferGasLimit
);

address public owner;
uint256 public lockCounter;
uint256 public transferGasLimit;

constructor() {
constructor(uint256 _transferGasLimit) Ownable(msg.sender) {
lockCounter = 1;
owner = msg.sender;
emit OwnerChange(address(0), owner);
transferGasLimit = 20000;
transferGasLimit = _transferGasLimit;
emit TransferGasLimitChange(0, transferGasLimit);
}

Expand All @@ -43,11 +40,6 @@ contract Batcher {
require(localCounter == lockCounter, 'Reentrancy attempt detected');
}

modifier onlyOwner() {
require(msg.sender == owner, 'Not owner');
_;
}

/**
* Transfer funds in a batch to each of recipients
* @param recipients The list of recipients to send to
Expand All @@ -67,17 +59,23 @@ contract Batcher {
);
require(recipients.length < 256, 'Too many recipients');

uint256 totalSent = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

will this be able to handle large values, since we will values in wei?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is generally used for denoting wei in solidity


// Try to send all given amounts to all given recipients
// Revert everything if any transfer fails
for (uint8 i = 0; i < recipients.length; i++) {
require(recipients[i] != address(0), 'Invalid recipient address');
emit BatchTransfer(msg.sender, recipients[i], values[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't get how this change solves reentrancy attacks (are we okay with other contracts making our contract methods, recursively)
Also incase the call fails, will we still emit events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the call function fails, the require(success, 'Send failed') statement will cause the transaction to revert. This will roll back the BatchTransfer event. And I think if the transaction reverts, the events and all state changes will also be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's kind of a common pattern to prevent reentrancy attacks i.e. to make the state updates before the external call. If a state variable was updated after the call, a reentrant call could potentially interact with the contract again before the state was updated, leading to unexpected behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we even store a state in this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pattern also applies to event emissions along with state updates

(bool success, ) = recipients[i].call{
value: values[i],
gas: transferGasLimit
}('');
require(success, 'Send failed');
emit BatchTransfer(msg.sender, recipients[i], values[i]);

totalSent += values[i];
}

require(totalSent == msg.value, 'Total sent out must equal total received');
Copy link
Contributor

Choose a reason for hiding this comment

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

will the whole tx fail if this is not satisfied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

}

/**
Expand All @@ -96,16 +94,6 @@ contract Batcher {
return returnData;
}

/**
* Transfers ownership of the contract ot the new owner
* @param newOwner The address to transfer ownership of the contract to
*/
function transferOwnership(address newOwner) external onlyOwner {
require(newOwner != address(0), 'Invalid new owner');
emit OwnerChange(owner, newOwner);
owner = newOwner;
}

/**
* Change the gas limit that is sent along with batched transfers.
* This is intended to protect against any EVM level changes that would require
Expand Down
30 changes: 15 additions & 15 deletions contracts/Forwarder.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pragma solidity 0.8.20;
import '@openzeppelin/contracts/token/ERC1155/IERC1155.sol';
import '@openzeppelin/contracts/token/ERC721/IERC721.sol';
import '@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol';
import '@openzeppelin/contracts/token/ERC1155/utils/ERC1155Receiver.sol';
import '@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol';
import './ERC20Interface.sol';
import './TransferHelper.sol';
import './IForwarder.sol';
Expand All @@ -12,7 +12,7 @@ import './IForwarder.sol';
* Contract that will forward any incoming Ether to the creator of the contract
*
*/
contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
contract Forwarder is IERC721Receiver, ERC1155Holder, IForwarder {
// Address to which any funds sent to this contract will be forwarded
address public parentAddress;
bool public autoFlush721 = true;
Expand All @@ -39,14 +39,14 @@ contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
return;
}

(bool success, ) = parentAddress.call{ value: value }('');
require(success, 'Flush failed');

// NOTE: since we are forwarding on initialization,
// we don't have the context of the original sender.
// We still emit an event about the forwarding but set
// the sender to the forwarder itself
emit ForwarderDeposited(address(this), value, msg.data);

(bool success, ) = parentAddress.call{ value: value }('');
require(success, 'Flush failed');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you move this to after the emit event.

Copy link
Contributor Author

@gianchandania gianchandania Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This was to address the point related to CHECK EFFECT INTERACTION PATTERN VIOLATED. The point was that a reentrancy attack can occur when the function in the contract makes an external call to another untrusted contract
before resolving any effects. To minimize the risk of event emission order manipulation, it's advisable to implement the check-effect-interaction pattern as extensively as possible. i.e. emitting events before making external calls.


/**
Expand Down Expand Up @@ -145,15 +145,15 @@ contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
}

/**
* @inheritdoc IERC1155Receiver
* @inheritdoc ERC1155Holder
*/
function onERC1155Received(
address _operator,
address _from,
uint256 id,
uint256 value,
bytes calldata data
) external virtual override returns (bytes4) {
bytes memory data
) public virtual override returns (bytes4) {
IERC1155 instance = IERC1155(msg.sender);
require(
instance.supportsInterface(type(IERC1155).interfaceId),
Expand All @@ -168,15 +168,15 @@ contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
}

/**
* @inheritdoc IERC1155Receiver
* @inheritdoc ERC1155Holder
*/
function onERC1155BatchReceived(
address _operator,
address _from,
uint256[] calldata ids,
uint256[] calldata values,
bytes calldata data
) external virtual override returns (bytes4) {
uint256[] memory ids,
uint256[] memory values,
bytes memory data
) public virtual override returns (bytes4) {
IERC1155 instance = IERC1155(msg.sender);
require(
instance.supportsInterface(type(IERC1155).interfaceId),
Expand Down Expand Up @@ -303,9 +303,9 @@ contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

same question as above

Copy link
Contributor Author

@gianchandania gianchandania Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is the same reason as mentioned here.

emit ForwarderDeposited(msg.sender, value, msg.data);
(bool success, ) = parentAddress.call{ value: value }('');
require(success, 'Flush failed');
emit ForwarderDeposited(msg.sender, value, msg.data);
}

/**
Expand All @@ -315,7 +315,7 @@ contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
public
view
virtual
override(ERC1155Receiver, IERC165)
override(ERC1155Holder, IERC165)
returns (bool)
{
return
Expand Down
9 changes: 5 additions & 4 deletions contracts/ForwarderFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import './Forwarder.sol';
import './CloneFactory.sol';

contract ForwarderFactory is CloneFactory {
address public implementationAddress;
address public immutable implementationAddress;

event ForwarderCreated(
address newForwarderAddress,
Expand All @@ -31,13 +31,14 @@ contract ForwarderFactory is CloneFactory {
bytes32 finalSalt = keccak256(abi.encodePacked(parent, salt));

address payable clone = createClone(implementationAddress, finalSalt);
Forwarder(clone).init(

Copy link
Contributor

Choose a reason for hiding this comment

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

same question

Copy link
Contributor Author

@gianchandania gianchandania Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Same as this.

emit ForwarderCreated(
clone,
parent,
shouldAutoFlushERC721,
shouldAutoFlushERC1155
);
emit ForwarderCreated(
clone,
Forwarder(clone).init(
parent,
shouldAutoFlushERC721,
shouldAutoFlushERC1155
Expand Down
8 changes: 4 additions & 4 deletions contracts/ForwarderFactoryV4.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import './CloneFactory.sol';
* @notice This contract will deploy new forwarder contracts using the create2 opcode
*/
contract ForwarderFactoryV4 is CloneFactory {
address public implementationAddress;
address public immutable implementationAddress;

/**
* @notice Event triggered when a new forwarder is deployed
Expand Down Expand Up @@ -67,14 +67,14 @@ contract ForwarderFactoryV4 is CloneFactory {
bytes32 finalSalt = keccak256(abi.encodePacked(parent, feeAddress, salt));

address payable clone = createClone(implementationAddress, finalSalt);
ForwarderV4(clone).init(
emit ForwarderCreated(
clone,
parent,
feeAddress,
shouldAutoFlushERC721,
shouldAutoFlushERC1155
);
emit ForwarderCreated(
clone,
ForwarderV4(clone).init(
parent,
feeAddress,
shouldAutoFlushERC721,
Expand Down
29 changes: 14 additions & 15 deletions contracts/ForwarderV4.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pragma solidity 0.8.20;
import '@openzeppelin/contracts/token/ERC1155/IERC1155.sol';
import '@openzeppelin/contracts/token/ERC721/IERC721.sol';
import '@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol';
import '@openzeppelin/contracts/token/ERC1155/utils/ERC1155Receiver.sol';
import '@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol';
import './ERC20Interface.sol';
import './TransferHelper.sol';
import './IForwarderV4.sol';
Expand All @@ -12,7 +12,7 @@ import './IForwarderV4.sol';
* @title ForwarderV4
* @notice This contract will forward any incoming Ether or token to the parent address of the contract
*/
contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
contract ForwarderV4 is IERC721Receiver, ERC1155Holder, IForwarderV4 {
/// @notice Any funds sent to this contract will be forwarded to this address
address public parentAddress;
/// @notice Address which is allowed to call methods on this contract alongwith the parentAddress
Expand Down Expand Up @@ -89,16 +89,15 @@ contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
return;
}

(bool success, ) = parentAddress.call{ value: value }('');
require(success, 'Flush failed');

/**
* Since we are forwarding on initialization,
* we don't have the context of the original sender.
* We still emit an event about the forwarding but set
* the sender to the forwarder itself
*/
emit ForwarderDeposited(address(this), value, msg.data);
(bool success, ) = parentAddress.call{ value: value }('');
require(success, 'Flush failed');
}

/**
Expand Down Expand Up @@ -174,15 +173,15 @@ contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
}

/**
* @inheritdoc IERC1155Receiver
* @inheritdoc ERC1155Holder
*/
function onERC1155Received(
address _operator,
address _from,
uint256 id,
uint256 value,
bytes calldata data
) external virtual override returns (bytes4) {
bytes memory data
) public virtual override returns (bytes4) {
IERC1155 instance = IERC1155(msg.sender);
require(
instance.supportsInterface(type(IERC1155).interfaceId),
Expand All @@ -197,15 +196,15 @@ contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
}

/**
* @inheritdoc IERC1155Receiver
* @inheritdoc ERC1155Holder
*/
function onERC1155BatchReceived(
address _operator,
address _from,
uint256[] calldata ids,
uint256[] calldata values,
bytes calldata data
) external virtual override returns (bytes4) {
uint256[] memory ids,
uint256[] memory values,
bytes memory data
) public virtual override returns (bytes4) {
IERC1155 instance = IERC1155(msg.sender);
require(
instance.supportsInterface(type(IERC1155).interfaceId),
Expand Down Expand Up @@ -359,9 +358,9 @@ contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
return;
}

emit ForwarderDeposited(msg.sender, value, msg.data);
(bool success, ) = parentAddress.call{ value: value }('');
require(success, 'Flush failed');
emit ForwarderDeposited(msg.sender, value, msg.data);
}

/**
Expand All @@ -371,7 +370,7 @@ contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
public
view
virtual
override(ERC1155Receiver, IERC165)
override(ERC1155Holder, IERC165)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see in some places we have override(ERC1155Holder, IERC165) and only override
what is the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When override is used without any parentheses, it means that the function overrides a function from a single unspecified base contract. Solidity will look through all the base contracts to find a function that matches the name and parameters of the overriding function.

When override(ERC1155Holder, IERC165) is used, it means that the function overrides functions from the specified base contracts ERC1155Holder and IERC165. This is used when multiple base contracts have functions with the same name and parameters, and you want to specify which ones are being overridden. This is a requirement since Solidity 0.6.0 to avoid ambiguity.

returns (bool)
{
return
Expand Down
6 changes: 1 addition & 5 deletions contracts/TransferHelper.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ library TransferHelper {
(bool success, bytes memory returndata) = token.call(
abi.encodeWithSelector(0x23b872dd, from, to, value)
);
Address.verifyCallResult(
success,
returndata,
'TransferHelper::transferFrom: transferFrom failed'
);
Address.verifyCallResult(success, returndata);
Copy link
Contributor

Choose a reason for hiding this comment

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

was this a change in the original safetransferfrom function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because of a change shown here in Address.sol openzeppelin contract that we are using

}
}
4 changes: 2 additions & 2 deletions contracts/WalletFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import './WalletSimple.sol';
import './CloneFactory.sol';

contract WalletFactory is CloneFactory {
address public implementationAddress;
address public immutable implementationAddress;

event WalletCreated(address newWalletAddress, address[] allowedSigners);

Expand All @@ -19,7 +19,7 @@ contract WalletFactory is CloneFactory {
bytes32 finalSalt = keccak256(abi.encodePacked(allowedSigners, salt));

address payable clone = createClone(implementationAddress, finalSalt);
WalletSimple(clone).init(allowedSigners);
emit WalletCreated(clone, allowedSigners);
WalletSimple(clone).init(allowedSigners);
}
}
Loading
Loading