Function getPastDelegate
may return incorrect values
#87
Labels
3 (High Risk)
Assets can be stolen/lost/compromised directly
bug
Something isn't working
disagree with severity
Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments)
duplicate
This issue or pull request already exists
invalid
This doesn't seem right
sponsor confirmed
Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Lines of code
https://github.com/code-423n4/2022-03-paladin/blob/9c26ec8556298fb1dc3cf71f471aadad3a5c74a0/contracts/HolyPaladinToken.sol#L976-L990
Vulnerability details
Impact
By calling
delegate
function ofHolyPaladinToken
contract two or more times in a single block (including a transaction so it is possible to flash loan assets and pass them to more and more accounts), users can add elements to thedelegateCheckpoints
array with the samefromBlock
properties, but different delegatees. This edge case is not considered in the implementation ofgetPastDelegate
function, which returns a value as soon as it finds aDelegateCheckpoint
with matchingfromBlock
property (see HolyPaladinToken.sol:981-983). If there are more checkpoints with the same block number, that function should return the latest one, i.e. the one with the highest index.In its current implementation
getPastDelegate
function can also return a different value after changing the length of the log, either from the correct value to an incorrect or the other way.Any governance system based on reading this function will inevitably fail because the attacker can use voting power for their funds multiple times from different addresses.
Not only does this bug break any external tools which rely on that function, but it also affects the return value of
getPastVotes
function, which sometimes may not take the locking bonus into account.Recommended mitigation steps
There are two obvious ways of fixing this issue: either by removing the check in HolyPaladinToken.sol:981-983 or by not allowing adding elements to the
delegateCheckpoints
array with the samefromBlock
property. While both ways are viable, the latter one is preferred as it is a gas optimization.Checks should be added whether or not the block number of the last element of
delegateCheckpoints
array is equal to the current block number, and if it is, modify that element instead of adding a new one.The text was updated successfully, but these errors were encountered: