Zellic - Audit Reports

Category: Coding Mistakes

Deletion does not delete all values

Informational Severity

Informational Impact

Low Likelihood

Description

In VotingControllerStorageUpg, _removePool deletes poolData, but the slopeChanges mapping within poolData is not actually deleted. Currently, since poolData is always checked for activity when used, this does not pose a security risk.

Impact

Using delete on a (possibly nested) struct containing a mapping member does not delete the mapping. The following proof-of-concept script demonstrates that the slopeChanges mapping within poolData is not actually deleted.

contract AuditTest is Test {
    address public admin;
    address public user1;
    
    // for locking token
    ProxyAdmin public proxyAdminLockingToken;
    TransparentUpgradeableProxy public proxyLockingToken;
    LockingToken public lpLocking;
    MockERC20 public lockedToken;

    // for voting controller
    ProxyAdmin public proxyAdminAnzenVotingControllerUpg;
    TransparentUpgradeableProxy public proxyAnzenVotingControllerUpg;
    AnzenVotingControllerUpg public anzenVotingControllerUpg;
    MockERC20 public anzenToken;

    function setUp() public {
        admin = vm.addr(0x1);
        user1 = vm.addr(0x2);

        setUp_LockingToken();
        setUp_VotingController();        
    }

    function setUp_LockingToken() internal {
        vm.startPrank(admin);
        lockedToken = new MockERC20("Locked Token", "LCK");

        proxyAdminLockingToken = new ProxyAdmin();
        proxyLockingToken = new TransparentUpgradeableProxy(address(new LockingToken()), address(proxyAdminLockingToken), "");
        lpLocking = LockingToken(address(proxyLockingToken));
        lpLocking.initialize(
            "Anzen LP Locking",
            "LockLP",
            address(lockedToken),
            100000000,
            address(0),
            address(0)
        );

        lockedToken.mint(user1, 100 ether);
        vm.stopPrank();
    }

    function setUp_VotingController() internal {
        vm.startPrank(admin);
        anzenToken = new MockERC20("Anzen Token", "ANZ");

        proxyAdminAnzenVotingControllerUpg = new ProxyAdmin();
        proxyAnzenVotingControllerUpg = new TransparentUpgradeableProxy(address(new AnzenVotingControllerUpg(address(anzenToken), address(0x1337), 0)), address(proxyAdminAnzenVotingControllerUpg), "");
        anzenVotingControllerUpg = AnzenVotingControllerUpg(address(proxyAnzenVotingControllerUpg));
        anzenVotingControllerUpg.initialize();
        vm.stopPrank();
    }

    function testAuditRemovePoolDirty() public {
        vm.startPrank(admin);

        // write to PoolData storage slot
        address pool = address(0x1337);
        anzenVotingControllerUpg.addPool(
            0,
            pool,
            100,
            100
        );
        address[] memory pools = new address[](1);
        uint64[] memory weights = new uint64[](1);
        pools[0] = pool;
        weights[0] = 100;
        anzenVotingControllerUpg.vote(pools, weights);
        anzenVotingControllerUpg.removePool(address(0x1337));

        // check slopeChanges mapping slot dirty
        uint128[] memory wTimes = new uint128[](1);
        wTimes[0] = 62899200;
        (,,,uint128[] memory dirty) = anzenVotingControllerUpg.getPoolData(address(0x1337), wTimes);

        console.log("pool slopeChanges slot dirty: ", dirty[0]);
        vm.stopPrank();
    }
}

The following text is the result of the proof-of-concept script:

[PASS] testAuditRemovePoolDirty() (gas: 313463)
Logs:
  pool slopeChanges slot dirty:  15

Recommendations

Consider adding an array to record the keys of the nested map slopeChanges, and iterate over the array to delete all values in the nested map when performing deletion.

Remediation

Anzen Labs Inc. provided the following response:

The team decided to not make changes for those issues at this time