`votes[to]` mapping anchor time not adjusted correctly in `Equity.adjustRecipientVoteAnchor()` can lead to unexpected results

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L161

Vulnerability details

[H-01] votes[to] mapping anchor time not adjusted correctly in Equity.adjustRecipientVoteAnchor() can lead to unexpected results

Proof of Concept

Equity.sol#L161

function adjustRecipientVoteAnchor(address to, uint256 amount) internal returns (uint256){
    if (to != address(0x0)) {
        uint256 recipientVotes = votes(to); // for example 21 if 7 shares were held for 3 blocks
        uint256 newbalance = balanceOf(to) + amount; // for example 11 if 4 shares are added
        voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance); // new example anchor is only 21 / 11 = 1 block in the past
        return recipientVotes % newbalance; // we have lost 21 % 11 = 10 votes
    } else {
        // optimization for burn, vote anchor of null address does not matter
        return 0;
    }
}

In the Equity.sol contract, all recorded times are shifted by 24 bits to account for a sub-block time resolution of 24 bits.

voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance); 

However, in Equity.adjustRecipientVoteAnchor(), the voteAnchor mapping is incorrectly assigned to simply subtracting a decimal value of recipientVotes/newBalance instead of the correctly shifted value by 24 bits. This can lead to unexpected results as vote anchor of recipient is moved forward by alot lesser than expected.

This can mean the following:

-Not passing canRedeem checks when supposed to, leading to share holders unable to receive ZCHF tokens.

-Overestimation of lostVotes via adjustTotalVotes during moving of tokens (minting/transferring). This could lead to lower than expected totalVotesAtAnchor, leading to lower than expected Equity.totalVotes() calculated in system

-Underestimation of votes that holder (recipient) of tokens have. This can inturn lead to failing Equity.checkQualified() function even when they have enough votes. Prevents qualified share holders from denying minters, freshly proposed position and ability to restructure system.

For example, take the following scenario where anchorTime() = 3 << 24 and scenario of user trying to reedeem ZCHF tokens after minimum holding duration of 5 blocks.

1.Alice have 7 shares and held it for 3 blocks 2.Based on Equity.votes(address holder), she will have 21 shares (7 * 3 << 24) 3.Bob transfers alice 4 ZCHF tokens, so alice's new balance is 11 Tokens 4.To make sure votes of Alice does not change due to higher balance, we shift vote anchor forard but here we only shift it by 21/11 = 1 (rounddown). That is voteAnchor[to] = (3 << 24) - 1 = 50331647 instead of (3 << 24) - (1 << 24) = 33554432 5.Assume some time has passed and no minting or transferring to Alice has occured, now anchorTime = 7 << 24 (5 blocks). Assume minimum holding duration is 5 blocks. Alice should be able to redeem her ZCHF tokens given canReedem() will return 7 << 24 - 2 << 24 = 83886080 which is equal to 83886080. However, it will instead calculate 7 << 24 - 50331647 = 67108865 (4 blocks) which is less than the minimum 5 blocks required, blocking Alice from redeeming shares when she is qualified.

Tools Used

Manual Analysis

Recommendation

Shift bits accordingly to correctly assign voteAnchor mapping

function adjustRecipientVoteAnchor(address to, uint256 amount) internal returns (uint256){
    if (to != address(0x0)) {
        uint256 recipientVotes = votes(to); // for example 21 if 7 shares were held for 3 blocks
        uint256 newbalance = balanceOf(to) + amount; // for example 11 if 4 shares are added
        voteAnchor[to] = uint64(anchorTime() - ((recipientVotes / newbalance) << 24)); // new example anchor is only 21 / 11 = 1 block in the past
        return recipientVotes % newbalance; // we have lost 21 % 11 = 10 votes
    } else {
        // optimization for burn, vote anchor of null address does not matter
        return 0;
    }
}

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[0xA5DF]

I think recipientVotes is already shifted + no PoC

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid