TreasuryV1 Manual Review Findings

TV1-01M: Inexistent Prevention of Accidental Transfers

Type	Severity	Location
Input Sanitization		TreasuryV1.sol:L107-L114

Description:

The TreasuryV1::donate function does not prohibit native funds from being transferred when an EIP-20 asset donation is meant to be made.

Impact:

It is presently possible to accidentally transfer native funds to the TreasuryV1 contract without crediting them to a particular cause requiring manual intervention of a manager.

Example:

Copy
94/// @inheritdoc ITreasury
95function donate(
96  bytes32 cause,
97  address token,
98  uint256 amount
99) public payable override {
100  require(amount > 0, "TreasuryV1: Cannot donate 0 tokens");
101
102  uint256 increase;
103
104  if (token == ETH_ADDRESS) {
105    require(amount == msg.value, "TreasuryV1: Message value mismatch");
106    increase = msg.value;
107  } else {
108    IERC20 ercToken = IERC20(token);
109    uint256 original = ercToken.balanceOf(address(this));
110
111    IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
112
113    increase = ercToken.balanceOf(address(this)) - original;
114  }
115
116  _changeToken(cause, token, increase, true);
117
118  emit Donated(cause, token, increase);
119}

Recommendation:

We advise such a check to be imposed in the else branch of the if construct within TreasuryV1::donate, ensuring that msg.value is 0 in such a case.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The else branch of the code now properly validates that the msg.value is zero when an EIP-20 donation is made, alleviating this exhibit.

TV1-02M: Incorrect Order of Execution

Type	Severity	Location
Language Specific		TreasuryV1.sol:L133-L137, L139

Description:

The TreasuryV1::spend function contains an incorrect order of execution as it will first transfer the funds outward and then decrement them from storage permitting re-entrancy attacks to occur.

Impact:

A native fund transfer is performed with an uncapped amount of gas thus permitting a re-entrancy to spend more funds than should be possible for a particular cause as its TreasuryV1::tokenBalance evaluation would be outdated during the re-entrancy.

Example:

Copy
121/// @inheritdoc ITreasury
122function spend(
123  bytes32 cause,
124  address token,
125  uint256 amount,
126  address to
127) public override onlyRole(MANAGER_ROLE) {
128  require(
129    tokenBalance(cause, token) >= amount,
130    "TreasuryV1: Not enough tokens in cause"
131  );
132
133  if (token == ETH_ADDRESS) {
134    payable(to).call{value: amount}("");
135  } else {
136    IERC20(token).safeTransfer(to, amount);
137  }
138
139  _changeToken(cause, token, amount, false);
140
141  emit Spent(cause, token, to, amount);
142}

Recommendation:

We advise the code to invoke TreasuryV1::_changeToken before performing the conditional transfers, ensuring that the system's state is correct during any potential re-entrancy that may occur.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The TreasuryV1::_changeToken call was relocated prior to the disbursement of funds, alleviating this exhibit.

TV1-03M: Incorrect Setting Mechanism

Type	Severity	Location
Logical Fault		TreasuryV1.sol:L176, L177, L180-L189

Description:

The TreasuryV1::_checkSet function is meant to keep track of which tokens are present with a non-zero value within the global and the local cause set, however, it does so incorrectly as the function contains incorrect logical statements.

Impact:

The token.set status and each cause's tokens entry will be malformed in the current implementation.

Example:

Copy
159function _changeToken(
160  bytes32 cause,
161  address tokenAddress,
162  uint256 amount,
163  bool adding
164) internal {
165  TokenInfo storage token = _cause[cause].token[tokenAddress];
166  TokenInfo storage global = _globalCause.token[tokenAddress];
167
168  if (adding) {
169    token.balance += amount;
170    global.balance += amount;
171  } else {
172    token.balance -= amount;
173    global.balance -= amount;
174  }
175
176  _checkSet(token, _cause[cause], tokenAddress);
177  _checkSet(global, _globalCause, tokenAddress);
178}
179
180function _checkSet(
181  TokenInfo storage token,
182  Cause storage cause,
183  address tokenAddress
184) internal {
185  if (token.balance == 0 && token.set) {
186    token.set = true;
187    cause.tokens.push(tokenAddress);
188  }
189}

Recommendation:

We advise the TreasuryV1::_checkSet function to be updated, setting the current value to false in the if clause and removing the tokenAddress from the cause.tokens.

As a next step, an else branch should be introduced that evaluates whether the token.balance is greater-than zero and whether token.set has not been set yet (i.e. is false). In such a case, it should replicate the current statements setting the set status to true and adding the tokenAddress to the cause.tokens set.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The code of the TreasuryV1::_checkSet function was updated according to our recommendation and an additional safety measure was put in place in the form of the AddressSet instead of an array representing the tokens of a cause. As a result of these adjustments, we consider this exhibit fully alleviated.