Contract fails to deliver promised returns, but doesn't lose value
Description
Brief/Intro
Due to referential assignment of struct variables, the block interpolation is done incorrectly. Since the blocknumbers of that struct are currently not used this does not have a direct impact, beyond storing incorrect values (which may cause issues on future use)
Vulnerability Details
The VotingEscrow._checkpoint function does this assignment:
Point memory initialLastPoint = lastPoint;
This behaves like references in other languages such as Java, where if you manipulate a property on either variables, you would change the data on the underlying instance and both variables would then use the new value. A minimal foundry test to showcase this:
The term _time - initialLastPoint.ts will now always be 0 and lastPoint.blk always be calculated incorrectly (at least if _checkpoint has not been called for more than a week).
A POC that logs the resulting values:
// tested on fork number 19771835
function testMinhCheckpointing() public {
uint256 week = 1 weeks;
uint256 blocksPerWeek = 7*24*60*5; //@note assuming 1 block per 12 secs
veALCX.checkpoint();
vm.warp(block.timestamp + 3 * week);
vm.roll(block.number + 3 * blocksPerWeek);
veALCX.checkpoint();
console2.log("point.blk for epoch 2:", veALCX.getPointHistory(2).blk);
console2.log("point.blk for epoch 3:", veALCX.getPointHistory(3).blk);
console2.log("point.blk for epoch 4:", veALCX.getPointHistory(4).blk);
console2.log("point.blk for epoch 5:", veALCX.getPointHistory(5).blk);
}
This will log the following:
point.blk for epoch 2: 19771835
point.blk for epoch 3: 19771835
point.blk for epoch 4: 19771835
point.blk for epoch 5: 19923035
Note how the blk value only changes on the last iteration.
Impact Details
As mentioned in the intro for now the impact is limited to incorrect calculations and storage of the result. The blk field is currently not in use, but is referenced in the unused _findBlockEpoch method, which would be affected on future use.
Recommendation
Direct assignment doesnt copy, thats why Velodrome creates a new struct object explicitly: