ERC20Pull Manual Review Findings

ERC-01M: Potential Function Misuse

Type	Severity	Location
Logical Fault		ERC20Pull.sol:L73-L75

Description:

The linked function is set as external yet does not impose any access control permitting it to be misused hurting the sender of assets.

Example:

Copy
67/// Attempts to transfer `amount_` of `token` to this contract.
68/// Relies on `token` having been approved for at least `amount_` by the
69/// `sender`. Will revert if the transfer fails due to `safeTransferFrom`.
70/// Also relies on `token` not being malicious.
71/// @param amount_ The amount to attempt to pull to the implementing
72/// contract.
73function pullERC20(uint256 amount_) external {
74    IERC20(token).safeTransferFrom(sender, address(this), amount_);
75}

Recommendation:

We advise either its visibility to be adjusted or it to enforce some form of access control as it currently allows malicious parties to siphon arbitrary funds present in the sender account depending on how much approval has been set to the contract. Given that a common use case would be an infinite approval to the contract, this function would allow grieving to occur whereby a malicious party repeatedly invokes it whenever the sender receives token units and leads to loss of funds as they remain untracked in the implementation.

Alleviation:

After discussion with the Rain Protocol team, we concluded that this is an intended functionality for the contract and instead a check was introduced in the initialization of it that ensures the sender address represents a contract implementation and cannot be an EOA thereby ensuring that the approvals are set via code rather than arbitrarily by user interaction. As a result of these, we consider the exhibit dealt with.

ERC-02M: Mutability Documentation Discrepancy

Type	Severity	Location
Standard Conformity		ERC20Pull.sol:L19, L53-L65

Description:

The documentation of the ERC20Pull contract indicates that the addresses of sender and token are meant to be known at the construction time of a derivative implementation, however, no such guarantee is imposed as the variables are set in a dedicated initializeERC20Pull function rather than an actual constructor.

Example:

Copy
16/// @title ERC20Pull
17/// @notice Enables a contract to pull (transfer to self) some `IERC20` token
18/// from a sender. Both the sender and token must be known and trusted by the
19/// implementing contract at construction time, and are immutable.
20///
21/// This enables the `sender` to merely approve the implementing contract then
22/// anon can call `pullERC20` to have those tokens transferred. In some cases
23/// (e.g. distributing the proceeds of a raise) it is safer to only approve
24/// tokens than to transfer (e.g. if there is some bug reverting transfers).
25///
26/// The `sender` is singular and bound at construction to avoid the situation
27/// where EOA accounts inadvertantly "infinite approve" and lose their tokens.
28///
29/// The token is singular and bound at construction to avoid the situation
30/// where anons can force the implementing contract to call an arbitrary
31/// external contract.
32contract ERC20Pull {
33    using SafeERC20 for IERC20;
34
35    /// Emitted during initialization.
36    event ERC20PullInitialize(
37        /// `msg.sender` of initialize.
38        address sender,
39        /// Address that token can be pulled from.
40        address tokenSender,
41        /// Token that can be pulled.
42        address token
43    );
44
45    /// The `sender` that this contract will attempt to pull tokens from.
46    address private sender;
47    /// The ERC20 token that this contract will attempt to pull to itself from
48    /// `sender`.
49    address private token;
50
51    /// Initialize the sender and token.
52    /// @param config_ `ERC20PullConfig` to initialize.
53    function initializeERC20Pull(ERC20PullConfig memory config_) internal {

Recommendation:

We advise a constructor to be utilized as it permits the usage of the special immutable keyword in the declarations of sender and token, greatly optimizing the gas cost of all functions that utilize them. In any case, we advise the terminology of the documentation to be adjusted as the word immutable is a special keyword not applicable in the current implementation. We should note that constructor implementations setting immutable variables are completely compatible with upgrade-able contracts given that immutable variables do not occupy storage space.

Alleviation:

The terminology of the documentation was adjusted as per our recommendation indicating that the values of these members are meant to be known during initialization rather than construction. Given that the contract utilizes a factory pattern, declaring the variables as immutable would defeat its purpose. Thus, we consider this exhibit adequately dealt with.