Skip to content

Instantly share code, notes, and snippets.

@yuriy77k
Forked from danbogd/ETH_TetherUSD_audit_report.md
Created July 7, 2019 09:25
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 1 You must be signed in to fork a gist
  • Save yuriy77k/d75e9365b6ec7eefd46a237d09e673bc to your computer and use it in GitHub Desktop.
Save yuriy77k/d75e9365b6ec7eefd46a237d09e673bc to your computer and use it in GitHub Desktop.

Tether USD audit report.

1. Summary

This document is a security audit report performed by danbogd, where Tether USD has been reviewed.

2. In scope

Сommit hash .

3. Findings

In total, 9 issues were reported including:

  • 0 medium severity issues
  • 4 low severity issues
  • 5 owner privileges (ability of owner to manipulate contract, may be risky for investors)..
  • 0 notes.

No critical security issues were found.

3.1. Known vulnerabilities of ERC-20 token

Severity: low

Description

  1. It is possible to double withdrawal attack. More details here.

  2. Lack of transaction handling mechanism issue. WARNING! This is a very common issue and it already caused millions of dollars losses for lots of token users! More details here.

Recommendation

Add into a function transfer(address _to, ... ) following code:

require( _to != address(this) );

3.2. Owner Privileges

Severity: owner previliges

Description

Contract owner allow himself to:

  • set to zero balance of any user using addBlackList and destroyBlackFunds mechanism.

Line: 294.

    function destroyBlackFunds (address _blackListedUser) public onlyOwner {
    require(isBlackListed[_blackListedUser]);
    uint dirtyFunds = balanceOf(_blackListedUser);
    balances[_blackListedUser] = 0;
    _totalSupply -= dirtyFunds;
    DestroyedBlackFunds(_blackListedUser, dirtyFunds);
    }
  • pause/unpaused transfer functions;

Line: 238

modifier whenNotPaused() { require(!paused); _; }

  • Owner can upgrade contract and implement any logic in the new contract. And even if the new contract will be audited, at any time possible to change the address of the new contract again to not audited and insecure.

Line: 387.

    function deprecate(address _upgradedAddress) public onlyOwner {
    deprecated = true;
    upgradedAddress = _upgradedAddress;
    Deprecate(_upgradedAddress);
    }
  • can issue any amount of tokens (over totalSupply).

Line: 406.

    function issue(uint amount) public onlyOwner {
    require(_totalSupply + amount > _totalSupply);
    require(balances[owner] + amount > balances[owner]);

    balances[owner] += amount;
    _totalSupply += amount;
    Issue(amount);
    }
  • change at any time parameters of Maxfee and asisPointsRate.

Line: 429.

    function setParams(uint newBasisPoints, uint newMaxFee) public onlyOwner {
    require(newBasisPoints < 20);
    require(newMaxFee < 50);

    basisPointsRate = newBasisPoints;
    maximumFee = newMaxFee.mul(10**decimals);

    Params(basisPointsRate, maximumFee);
}

3.3. ERC20 Compliance — approve issues

Severity: low/medium

Description

There is no way to reset approved value to 0, because approve function contains:

require(!((_value != 0) && (allowed[msg.sender][_spender] != 0))); throw

Also it breaks the EIP20 security recommendation:

To prevent attack vectors like the one described [here](https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit) and discussed [here](https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729), clients SHOULD make sure to create user interfaces in such a way that they set the allowance first to 0 before setting it to another value for the same spender. THOUGH The contract itself shouldn't enforce it, to allow backwards compatibility with contracts deployed before

Code snippet

Line: 205

3.4. Poor workaround of Short Address Attack

Severity: low

Description

onlyPayloadSize modifier is workaround to avoid the Short Address Attack. But it doesn't work properly.

This method failed to execute when it was called from Parity multisignature wallet. The EVM pads call from this multisignature wallet, making the total 96 bytes instead of the expected 68.

If transfer and transferFrom are used by a subcontract function with fewer arguments, the onlyPayloadSize check will fail. It is not possible to adapt the workaround to prevent this issue.

More details here.

Code snippet

    modifier onlyPayloadSize(uint size) {
    require(!(msg.data.length < size + 4));
    _;
}

Recommendation

Remove this workaround.

3.5. ERC20 Compliance — event missing

Severity: low

Description

According to ERC20 standard when coins are minted a Transfer event should be emitted.

Code snippet

Line 335.

4. Conclusion

The review did not show any critical issues, some of low severity issues were found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment