Platform: Code4rena
Start Date: 25/11/2021
Pot Size: $80,000 USDC
Total HM: 35
Participants: 32
Period: 7 days
Judge: GalloDaSballo
Total Solo HM: 27
Id: 59
League: ETH
Rank: 13/32
Findings: 3
Award: $1,696.10
🌟 Selected for report: 5
🚀 Solo Findings: 0
298.0144 USDC - $298.01
gpersoon
The function bondToAccount() of Bonding.sol has a check based on _notSameBlock() _notSameBlock() makes sure the same msg.sender cannot do 2 actions within the same block.
However this can be circumvented in this case: Suppose you call bondToAccount() via a (custom) smart contract, then the msg.sender will be the address of the smart contract. For a pseudo code proof of concept see below.
I'm not sure what the deeper reason is for the _notSameBlock() in bondToAccount(). But if it is important then circumventing this check it will pose a risk.
call function attack1.attack()
contract attack1 { function attack(address account, uint256 amount) { call attack2.forward(account, amount); call any other function of malt } } contract attack2 { function forward(address account, uint256 amount) { call bonding.bondToAccount(account, amount); // uses msg.sender of attack2 } }
function bondToAccount(address account, uint256 amount) public { if (msg.sender != offering) { _notSameBlock(); } ...
function _notSameBlock() internal { require( block.number > lastBlock[_msgSender()],"Can't carry out actions in the same block" ); lastBlock[_msgSender()] = block.number; }
Add access controls to the function bondToAccount() An end-user could still call bond()
#0 - GalloDaSballo
2022-01-09T23:34:16Z
notSameBlock is effectively being used as the nonReentrant modifier, without the same security guarantees, as such, in spite of not having a specific attack vector, because the warden showed how to side step this security feature of the protocol, am going to raise the severity to Medium
496.6906 USDC - $496.69
gpersoon
The function setAdvanceIncentive of DAO.sol doesn't check for a maximum value of incentive. If incentivewould be very large, then advanceIncentive would be very large and the function advance() would mint a large amount of malt.
The function setAdvanceIncentive() can only be called by an admin, but a mistake could be made. Also if an admin would want to do a rug pull, this would be an ideal place to do it.
function setAdvanceIncentive(uint256 incentive) externalonlyRole(ADMIN_ROLE, "Must have admin role") { ... advanceIncentive = incentive;
function advance() external { ... malt.mint(msg.sender, advanceIncentive * 1e18);
Check for a reasonable maximum value in advance()
#0 - 0xScotch
2021-12-03T19:40:21Z
Definitely need to guard against arbitrarily large incentives. Disagree the risk is medium though.
#1 - GalloDaSballo
2022-01-22T15:07:17Z
Agree with the finding, this is an example of admin privilege, where the admin can set a variable which can be used to dilute the token and rug the protocol.
Because this is contingent on the admin's action, I believe medium severity to be proper
#2 - GalloDaSballo
2022-01-30T16:41:57Z
The simple rationale on the medium severity is that the owner could set the incentive to an exorbitant amount with the goal of minting a lot of tokens for an exit scam
🌟 Selected for report: gpersoon
367.919 USDC - $367.92
gpersoon
The function setSampleMemory of MovingAverage.sol takes the modulo of counter with the new value of _sampleMemory: "counter = counter % _sampleMemory;"
Suppose: counter =15 ; sampleMemory=10 and _sampleMemory=12 Then: counter = counter % _sampleMemory ==> 3, which means processing will continue at position 3.
However I think it should use: counter = counter % sampleMemory, so it will continue at position 5
function setSampleMemory(uint256 _sampleMemory) external onlyRole(ADMIN_ROLE, "Must have admin privs") { ... if (_sampleMemory > sampleMemory) { ... counter = counter % _sampleMemory; } else { } sampleMemory = _sampleMemory; }
Doublecheck the theory above and if you agree: change
counter = counter % _sampleMemory;
to
counter = counter % sampleMemory;
#0 - GalloDaSballo
2022-01-24T01:09:47Z
The sponsor agrees with the finding so we'll allow
🌟 Selected for report: gpersoon
367.919 USDC - $367.92
gpersoon
The function setStabilityThresholds of StabilizerNode.sol set the values for upperStabilityThreshold and lowerStabilityThreshold, however there is no check for a maximum value. This means that in function _shouldAdjustSupply() the values for upperThreshold and lowerThreshold could get larger than priceTarget. When they are subtracted from priceTarget a revert will occur.
Thus it is useful the make sure that upperStabilityThreshold and lowerStabilityThreshold don't get too large.
function setStabilityThresholds(uint256 _upper, uint256 _lower) external onlyRole(ADMIN_ROLE, "Must have admin role") { require(_upper > 0 && _lower > 0, "Must be above 0"); upperStabilityThreshold = _upper; lowerStabilityThreshold = _lower;
function _shouldAdjustSupply(uint256 exchangeRate) internal view returns (bool) { ... uint256 upperThreshold = priceTarget.mul(upperStabilityThreshold).div(10**decimals); // upperStabilityThreshold could be > 10**dec => upperThreshold could be > priceTarget uint256 lowerThreshold = priceTarget.mul(lowerStabilityThreshold).div(10**decimals); // lowerStabilityThreshold could be > 10**dec => lowerThreshold could be > priceTarget return (exchangeRate <= priceTarget.sub(lowerThreshold) && !auction.auctionActive(auction.currentAuctionId())) || exchangeRate >= priceTarget.add(upperThreshold); // can revert }
In function setStabilityThresholds() check for a maximum value of upperStabilityThreshold and lowerStabilityThreshold
#0 - GalloDaSballo
2022-01-25T01:39:30Z
Agree with the finding, as with any admin privilege, providing clear limits gives better guarantees to protocol users. Because there's no specific attack vector, I agree with low severity
165.5635 USDC - $165.56
gpersoon
The function setAuctionAverageLookback of AuctionBurnReserveSkew.sol change auctionAverageLookback However there is also the variable "count" that is used in amongst others, addAbovePegObservation(). The modulo of count with auctionAverageLookback is calculated via _getIndexOfObservation(). When you change auctionAverageLookback then the modulo will result in a different value, so you end up in a different location of the circular buffer.
You should probably adapt count as well in the function setAuctionAverageLookback() (see also function setSampleMemory of MovingAverage.sol where a similar pattern is used)
function setAuctionAverageLookback(uint256 _lookback) external onlyRole(ADMIN_ROLE, "Must have admin role") { .. if (_lookback > auctionAverageLookback) { for (uint i = auctionAverageLookback; i < _lookback; i++) { pegObservations.push(0); } } auctionAverageLookback = _lookback; ...
function addAbovePegObservation(uint256 amount) public onlyRole(STABILIZER_NODE_ROLE, "Must be a stabilizer node to call this method") { uint256 index = _getIndexOfObservation(count); ... pegObservations[index] = 1; count = count + 1; ...
function _getIndexOfObservation(uint _index) internal view returns (uint index) { return _index % auctionAverageLookback; }
Doublecheck the theory above and if you agree: Add the following statement in the function setAuctionAverageLookback(), before auctionAverageLookback is updated.
count = count % auctionAverageLookback ; // the old version of auctionAverageLookback
#0 - GalloDaSballo
2022-01-25T02:31:43Z
Sponsor confirms, technically the issue can be sidestepped by doing the math and then setting w/e value is needed, as such low severity is appropriate