akropolisio/akropolistoken

Akropolis Token

akropolistoken's Issues

Collision of storage layouts of TokenProxy and AkropolisToken

The problem is illustrated by the test (works for commit 3ad8eaa):

const BalanceSheet = artifacts.require("BalanceSheet")
const AllowanceSheet = artifacts.require("AllowanceSheet")
const AkropolisBaseToken = artifacts.require("AkropolisBaseToken")
const AkropolisToken = artifacts.require("AkropolisToken")
const TokenProxy = artifacts.require("TokenProxy")
const TokenProxyDelayed = artifacts.require("TokenProxyDelayed")

const ZERO_ADDRESS = 0x0000000000000000000000000000000000000000;
const l = console.log;


contract('TestProxySlotCollision', accounts => {
    const owner = accounts[0];

    async function instantiate() {
        // migration 2
        const balances = await BalanceSheet.new({from: owner});
        const allowances = await AllowanceSheet.new({from: owner});

        // migration 3
        const token = await AkropolisToken.new(ZERO_ADDRESS, ZERO_ADDRESS, {from: owner});

        // migration 4
        const proxy = await TokenProxy.new(token.address, balances.address, allowances.address, "Akropolis", 18, "AKT", {from: owner});

        return [token, proxy];
    }

    it('test collision', async function () {
        const [token, proxy] = await instantiate();

        let decimals = await proxy.decimals();
        l(decimals.toString());
        assert.equal(decimals, 18);

        const callData = token.enableWhitelist.request().params[0].data;
         // in essence, it's `proxy.enableWhitelist()` executed by the owner
        await web3.eth.sendTransaction({to: proxy.address, data: callData, from: owner});

        decimals = await proxy.decimals();
        l(decimals.toString());
        assert.equal(decimals, 18);
    })
})

The problem here is that TokenProxy and AkropolisToken (which code is executed over TokenProxy storage during delegatecall) have different storage layouts and they collide. It's a very dangerous practice to use "simple" storage variables in proxies (see solution below).

Storage layout of the TokenProxy (slot: variable):

0: balances
1: allowances
2: owner
3: pendingOwner
4: name
5: decimals
6: symbol
0x7050c9e0f4ca769c69bd3a8ef740bc37934f8e2c036e5a723fd8ee048ed3f8c3: implementation address

Storage layout as used by the AkropolisToken code (slot: variable):

0: balances
1: allowances
2: owner
3: packed by the compiler: pendingOwner, paused, locked
4: whitelist mapping (slot unused)
5: bool whitelisted

As can be seen, collision almost have't happened because paused and locked flags was packed by solidity compiler and don't collide with other fields and slot for whitelist is not used (because mappings are implemented in such way). But there is collision of bool whitelisted and decimals fields.

A simple solution is to use "unique" slot locations for each field (except shared base contract fields) derived via keccak256, for example: https://github.com/poanetwork/poa-network-consensus-contracts/blob/0c175cb98dac52201342f4e5e617f89a184dd467/contracts/KeysManager.sol#L185.
In this case we also recommend to include contract name into hash function invocation, and use abi.encode in place of abi.encodePacked, like this: uintStorage[keccak256(abi.encode("TokenProxy", "decimals"))] = decimals.

Contract for bounty users with confirmation

Unit tests for token functions

ERC20 Token Interface

Additional tests for bounty users

Basic structure

Token proxy: From the user’s point of view the proxy is the key to enabling flexible upgradeability.

┆Attachments: 1*PL4Y3tqS_7n4-funx7xNqw.jpeg

Migrations

PausableToken Implementation

AkropolisToken: return values of super calls are ignored

Affected code:

AkropolisToken/contracts/token/AkropolisToken.sol

Line 41 in 3ad8eaa

super.approve(_spender, _value);

AkropolisToken/contracts/token/AkropolisToken.sol

Line 75 in 3ad8eaa

super.transfer(_to, _amount);

AkropolisToken/contracts/token/AkropolisToken.sol

Line 92 in 3ad8eaa

super.transferFrom(_from, _to, _amount);

Result of base contract function calls are ignored and the functions in question always return false. Users of AkropolisToken contract which check function result (including other smart contracts, of course) will treat mentioned calls as failed. Looks like return super....(...) is missed.

Install truffle and dependencies

Whitelist for some pulls

UpgradeableToken Implementation

Additional tests for whitelists functions

LockableToken Implementation