When user unstake and decrease shares, these voting power is not decreased, as there is no mechanism to do that:
function unstake( uint256 amountUnstaked, uint256 numWeeks ) external nonReentrant returns (uint256 unstakeID)//@audit đánh giá sự thay đổi khi unstake -> update config -> xxx
{
require( userShareForPool(msg.sender, PoolUtils.STAKED_SALT) >= amountUnstaked, "Cannot unstake more than the amount staked" );
uint256 claimableSALT = calculateUnstake( amountUnstaked, numWeeks );
uint256 completionTime = block.timestamp + numWeeks * ( 1 weeks );
unstakeID = nextUnstakeID++;
Unstake memory u = Unstake( UnstakeState.PENDING, msg.sender, amountUnstaked, claimableSALT, completionTime, unstakeID );
_unstakesByID[unstakeID] = u;
_userUnstakeIDs[msg.sender].push( unstakeID );
// Decrease the user's staking share so that they will receive less future SALT rewards
// This call will send any pending SALT rewards to msg.sender as well.
// Note: _decreaseUserShare checks to make sure that the user has the specified staking share balance.
_decreaseUserShare( msg.sender, PoolUtils.STAKED_SALT, amountUnstaked, false );//@auditz stake & unstake không dùng cooldown (nó có cooldown trên kia kìa)
emit UnstakeInitiated(msg.sender, unstakeID, amountUnstaked, claimableSALT, numWeeks);
}
Attacker can abusing this to increase voting for free by keep voting, unstake, then vote again. This can be abused by attack to execute malicious actions. A way to execute malicious action is propose CALL_CONTRACT to malicious address that delegate call to multiple function that have onlyOwner function, like whitelistPools(), unwhitelistPool(), ....
Moreover, there is no expire for ballot, it will only check if ballot is executed before, passed minimum end time and it pass required quorum or not:
function canFinalizeBallot( uint256 ballotID ) external view returns (bool)
{
Ballot memory ballot = ballots[ballotID];
if ( ! ballot.ballotIsLive )
return false;
// Check that the minimum duration has passed
if (block.timestamp < ballot.ballotMinimumEndTime )
return false;
// Check that the required quorum has been reached
if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
return false;
return true;
}
Attack scenario:
Attacker call proposeCallContract with input is malicious contract address
Attacker vote for that proposal
Unstake salt token
Stake again by using another address have staking access
Cast vote for that ballot again
Repeat this process until vote is enough
This process can be extended for years, depend on number of vote attacker have and unstaking period. But when it is success, impact is very huge, since all functions that have onlyOwner can be delegate call from that malicious contract.
Impact
Attacker can freely edit configurations of many variable. But the most dangerous is, attacker can unwhitelist any token that restricted when calling proposeTokenUnwhitelisting() function, lead to protocol break. Moreover, attacker can whitelist multiple pools with their malicious token that only owned by them to steal salt rewards
Tools Used
Manual review
Recommended Mitigation Steps
There are multiple thing that need to fix:
Ballot should have expire time
When user unstake salt, all previous voting power should be loss
Propose to call to arbitrary contract should be deleted, because even when these 2 solution above is mitigrated, it still have risk that anyone have 51% voting power can control this protocol
Lines of code
https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L259-#L293 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Staking.sol#L60-#L79
Vulnerability details
Vulnerability detail
When cast vote on a open ballot, user's vote is calculated based on user's current share of salt:
When user unstake and decrease shares, these voting power is not decreased, as there is no mechanism to do that:
Attacker can abusing this to increase voting for free by keep voting, unstake, then vote again. This can be abused by attack to execute malicious actions. A way to execute malicious action is propose
CALL_CONTRACT
to malicious address that delegate call to multiple function that haveonlyOwner
function, likewhitelistPools()
,unwhitelistPool()
, ....Moreover, there is no expire for ballot, it will only check if ballot is executed before, passed minimum end time and it pass required quorum or not:
Attack scenario:
proposeCallContract
with input is malicious contract addressThis process can be extended for years, depend on number of vote attacker have and unstaking period. But when it is success, impact is very huge, since all functions that have
onlyOwner
can be delegate call from that malicious contract.Impact
Attacker can freely edit configurations of many variable. But the most dangerous is, attacker can unwhitelist any token that restricted when calling
proposeTokenUnwhitelisting()
function, lead to protocol break. Moreover, attacker can whitelist multiple pools with their malicious token that only owned by them to steal salt rewardsTools Used
Manual review
Recommended Mitigation Steps
There are multiple thing that need to fix:
Assessed type
Context