search

code-423n4 / 2022-08-fiatdao-findings

2 stars 1 forks source link

Improper require() with locked.end in `VotingEscrow.delegate()` #288

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-fiatdao/blob/fece3bdb79ccacb501099c24b60312cd0b2e4bb2/contracts/VotingEscrow.sol#L589

Vulnerability details

Impact

VotingEscrow.delegate() requires the delegated lock must have longer end here.

This require() might force the delegator to increase the locked.end to delegate back to himself for withdrawal.

Proof of Concept

It's required to check the longer end when the delegator sets the delegatee and changes to another one.

But when he delegates back to himself for withdrawal, I think we shouldn't check this require().

Otherwise, the delegator must increase his end all the time as the delegatee's end will be greater than his end.

Furthermore, it will be worse when the delegatee increased the end meanwhile.

Tools Used

Solidity Visual Developer of VSCode

Recommended Mitigation Steps

Recommend passing the require when _addr == msg.sender here.

if(_addr != msg.sender) {
    require(toLocked.end >= fromLocked.end, "Only delegate to longer lock");
}
lacoop6tu commented 2 years ago

This is an expected behaviour

lacoop6tu commented 2 years ago

Duplicate of #188